Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle button #336

Merged
merged 27 commits into from
Feb 8, 2022
Merged

Toggle button #336

merged 27 commits into from
Feb 8, 2022

Conversation

mollykreis
Copy link
Contributor

Pull Request

🤨 Rationale

Create a toggle button within nimble-components

👩‍💻 Implementation

The new toggle button is built on top of the FAST switch but styled like the nimble-button.

The nimble-toggle-button and the nimble-button share appearance modes and styles, which are now sourced within patterns/buttons.

🧪 Testing

  • Manually tested the new buttons in storybook
  • Added automated tests that are largely based on the FAST tests for the switch
  • Verified there are no accessibility violations in story book for the new stories

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

packages/nimble-components/src/button/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/toggle-button/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/toggle-button/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/toggle-button/index.ts Outdated Show resolved Hide resolved
border-color: ${fillColorSelected};
}

.control[aria-pressed='true']:hover:not([disabled]) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the pattern here: https://github.com/ni/nimble/blob/main/packages/nimble-components/docs/css-guidelines.md#prefer-cascade-for-state-changes

Some ideas:

  • It looks like all of these styles are targeting the the .control element with aria-pressed='true' can we put that selector first?
  • Can we avoid using the :not() operator? I think it's easier to understand the style when the state is spelled out explicitly. I don't have to evaluate the list of selectors associated with the same properties or evaluate not selectors. It becomes more of a look-up table for specific states. ie something like the following (I didn't check thoroughly for accuracy, may need to double check the logic. I also re-ordered it to make hover < focusVisible < active like the docs show, but dunno if that's the intention here):
    .control[aria-pressed='true'] {
        background-color: ${fillColorSelected};
        border-color: ${fillColorSelected};
    }

    .control[aria-pressed='true']:hover {
        background-color: ${fillColorSelected};
        border-color: ${borderColorHover};
    }

    .control[aria-pressed='true']${focusVisible} {
        background-color: ${fillColorSelected};
    }

    .control[aria-pressed='true']:active {
        background-color: ${fillColorSelected};
        border-color: ${fillColorSelected};
    }


    .control[aria-pressed='true'][disabled] {
        background-color: ${fillColorSelected};
        border-color: ${fillColorSelected};
    }

    .control[aria-pressed='true'][disabled]:hover {
        background-color: ${fillColorSelected};
        border-color: ${fillColorSelected};
    }

Copy link
Member

Choose a reason for hiding this comment

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

When spelled out like that I wonder if background-color: ${fillColorSelected}; needs to be defined in each selector. Is it covered by the definition in the first .control[aria-pressed='true']? It could easily be due to other styles in buttonStyles that are being overriden in the cascade. Just thought I'd ask jic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reordered the styles and got rid of the :not() selector as you suggested. I confirmed that all the usages background-color: ${fillColorSelected} are required, but I was able to remove the hover border styling since it was duplicated from the underlying button styles.

packages/nimble-components/src/toggle-button/styles.ts Outdated Show resolved Hide resolved
@mollykreis mollykreis requested a review from rajsite February 8, 2022 18:39
@mollykreis mollykreis merged commit fbaa799 into main Feb 8, 2022
@mollykreis mollykreis deleted the toggle-button branch February 8, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants