Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 144 additions & 28 deletions components/button/button-styles.js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think would be better if you use the split view so you can see what was replaced by what

Original file line number Diff line number Diff line change
@@ -1,33 +1,149 @@
import '../colors/colors.js';
import { css } from 'lit';
import { css, unsafeCSS } from 'lit';
import { _isValidCssSelector } from '../../helpers/internal/css.js';
import { getFocusRingStyles } from '../../helpers/focus.js';

export const buttonStyles = css`
button {
border-end-end-radius: var(--d2l-button-end-end-radius, 0.3rem);
border-end-start-radius: var(--d2l-button-end-start-radius, 0.3rem);
border-start-end-radius: var(--d2l-button-start-end-radius, 0.3rem);
border-start-start-radius: var(--d2l-button-start-start-radius, 0.3rem);
border-style: none;
box-sizing: border-box;
cursor: pointer;
display: inline-block;
margin: 0;
min-height: calc(2rem + 2px);
outline: none;
text-align: center;
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
vertical-align: middle;
white-space: nowrap;
width: auto;
}
${getFocusRingStyles('button')}
@media (prefers-contrast: more) {
button {
border: 2px solid transparent;
function _generateButtonBaseStyles(selector) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

return css`
${selector} {
border-end-end-radius: var(--d2l-button-end-end-radius, 0.3rem);
border-end-start-radius: var(--d2l-button-end-start-radius, 0.3rem);
border-start-end-radius: var(--d2l-button-start-end-radius, 0.3rem);
border-start-start-radius: var(--d2l-button-start-start-radius, 0.3rem);
border-style: none;
box-sizing: border-box;
cursor: pointer;
display: inline-block;
margin: 0;
min-height: calc(2rem + 2px);
outline: none;
text-align: center;
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
vertical-align: middle;
white-space: nowrap;
width: auto;
}
`;
}

export function _generateButtonStyles(selector) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

return css`
${selector} {
font-family: inherit;
padding-block-end: 0;
padding-block-start: 0;
padding-inline-end: var(--d2l-button-padding-inline-end, 1.5rem);
padding-inline-start: var(--d2l-button-padding-inline-start, 1.5rem);
}
`;
}

export function _generateMozillaButtonBorderStyles(selector) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

return css`
/* Firefox includes a hidden border which messes up button dimensions */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of generator methods. Was hoping core would just have two - one for the base styles (defined in the mixin), and another for the d2l-button styles.

Can this be combined with the template immediately above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have done the following:

  1. I am not exposing them any more
  2. I am calling them within the _generateButtonStyles function
  3. I am only exposing the _generateButtonStyles function to generate d2l-button specific styles.
    I need these private functions later in order to generate the BSI styles reusing their logic (here is the PR for that, but this one must be merged first)

${selector}::-moz-focus-inner {
border: 0;
}
`;
}

export function _generateButtonDisabledStyles(selector, isForBsi = false) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here - do we need to support a special generator just for the disabled styles?

The BSI aspect creeping in here isn't great. If we really need to distinguish the context, I think we can make it more meaningful by naming it something like hasHost or forWebComponent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback....it's been a while since reviewing this so I will come up if some updates trying to put all together into a single generator function (2 tops as you suggested).

if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

const activeDisabledSelector = isForBsi ? unsafeCSS(`${selector}[active][disabled]`) : unsafeCSS(`:host([active]) ${selector}[disabled]`);

return css`
${selector},
${selector}[disabled]:hover,
${selector}[disabled]:focus,
${activeDisabledSelector} {
background-color: var(--d2l-theme-background-color-interactive-secondary-default);
color: var(--d2l-theme-text-color-static-standard);
}
`;
}

export function _generateButtonEnabledStyles(selector, isForBsi = false) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

const activeSelector = isForBsi ? unsafeCSS(`${selector}[active]`) : unsafeCSS(`:host([active]) ${selector}`);
const additionalBSISelector = isForBsi ? unsafeCSS(`${selector}.d2l-button-hover, ${selector}.d2l-button-focus,`) : unsafeCSS('');

return css`
${additionalBSISelector}
${selector}:hover,
${selector}:focus,
${activeSelector} {
background-color: var(--d2l-theme-background-color-interactive-secondary-hover);
}
`;
}

export function _generateBSIButtonFocusStyles(selector) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
const getSelector = (focusPseudoClass) => `
${selector}:${focusPseudoClass},
${selector}.d2l-button-focus,
${selector}[primary]:${focusPseudoClass},
${selector}[primary].d2l-button-focus`;

return getFocusRingStyles(getSelector);
}

export function _generatePrimaryButtonDisabledStyles(selector, isForBsi = false) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

const finalSelector = isForBsi ? `${selector}[primary],
${selector}[primary][disabled]:hover,
${selector}[primary][disabled]:focus,
${selector}[primary][active][disabled]` : `:host([primary]) ${selector},
:host([primary]) ${selector}[disabled]:hover,
:host([primary]) ${selector}[disabled]:focus,
:host([primary][active]) ${selector}[disabled]`;

return css`
${unsafeCSS(finalSelector)} {
background-color: var(--d2l-theme-background-color-interactive-primary-default);
color: var(--d2l-theme-text-color-static-inverted);
}
}
`;
}

export function _generatePrimaryButtonEnabledStyles(selector, isForBsi = false) {
if (!_isValidCssSelector(selector)) return unsafeCSS('');
selector = unsafeCSS(selector.trim());

const finalSelector = isForBsi ? `${selector}[primary]:hover,
${selector}[primary].d2l-button-hover,
${selector}[primary]:focus,
${selector}[primary].d2l-button-focus,
${selector}[primary][active]` : `:host([primary]) ${selector}:hover,
:host([primary]) ${selector}:focus,
:host([primary][active]) ${selector}`;

return css`
${unsafeCSS(finalSelector)} {
background-color: var(--d2l-theme-background-color-interactive-primary-hover);
}
`;
}

export const buttonStyles = css`
${_generateButtonBaseStyles('button')}
${getFocusRingStyles('button', { preferContrastMediaQueryExtraStyles: css`border: 2px solid transparent;` })}
`;
55 changes: 15 additions & 40 deletions components/button/button.js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deletes styles from this component are now being generated using the _generateButtonStyles('button') (line 10) function and also I use an alias (buttonStyles as baseButtonStyles) to be more accurate about each style

Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
import '../colors/colors.js';
import '../tooltip/tooltip.js';
import {
_generateButtonDisabledStyles,
_generateButtonEnabledStyles,
_generateButtonStyles,
_generateMozillaButtonBorderStyles,
_generatePrimaryButtonDisabledStyles,
_generatePrimaryButtonEnabledStyles,
buttonStyles
} from './button-styles.js';
import { css, html, LitElement } from 'lit';
import { ButtonMixin } from './button-mixin.js';
import { buttonStyles } from './button-styles.js';
import { getUniqueId } from '../../helpers/uniqueId.js';
import { ifDefined } from 'lit/directives/if-defined.js';
import { labelStyles } from '../typography/styles.js';
Expand Down Expand Up @@ -35,35 +43,13 @@ class Button extends ButtonMixin(LitElement) {
:host([hidden]) {
display: none;
}

${_generateButtonStyles('button')}
button {
font-family: inherit;
padding-block-end: 0;
padding-block-start: 0;
padding-inline-end: var(--d2l-button-padding-inline-end, 1.5rem);
padding-inline-start: var(--d2l-button-padding-inline-start, 1.5rem);
width: 100%;
}

/* Firefox includes a hidden border which messes up button dimensions */
button::-moz-focus-inner {
border: 0;
}

button,
button[disabled]:hover,
button[disabled]:focus,
:host([active]) button[disabled] {
background-color: var(--d2l-theme-background-color-interactive-secondary-default);
color: var(--d2l-theme-text-color-static-standard);
}

button:hover,
button:focus,
:host([active]) button {
background-color: var(--d2l-theme-background-color-interactive-secondary-hover);
}

${_generateMozillaButtonBorderStyles('button')}
${_generateButtonDisabledStyles('button')}
${_generateButtonEnabledStyles('button')}
:host([disabled]) button {
cursor: default;
position: relative;
Expand All @@ -76,19 +62,8 @@ class Button extends ButtonMixin(LitElement) {
opacity: var(--d2l-theme-opacity-disabled-control);
position: absolute;
}

:host([primary]) button,
:host([primary]) button[disabled]:hover,
:host([primary]) button[disabled]:focus,
:host([primary][active]) button[disabled] {
background-color: var(--d2l-theme-background-color-interactive-primary-default);
color: var(--d2l-theme-text-color-static-inverted);
}
:host([primary]) button:hover,
:host([primary]) button:focus,
:host([primary][active]) button {
background-color: var(--d2l-theme-background-color-interactive-primary-hover);
}
${_generatePrimaryButtonDisabledStyles('button')}
${_generatePrimaryButtonEnabledStyles('button')}
`
];

Expand Down