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

Prefer attributes over CSS classes #738

Merged
merged 55 commits into from
Sep 29, 2022
Merged

Prefer attributes over CSS classes #738

merged 55 commits into from
Sep 29, 2022

Conversation

rajsite
Copy link
Member

@rajsite rajsite commented Sep 13, 2022

Pull Request

🤨 Rationale

This PR migrates nimble away from using host CSS classes as part of the API for nimble components.

For users, host CSS classes defining styles provides poor typing compared to attributes and is strange as users expect to define classes and associated styles themselves. For nimble developers host classes make inflexible APIs as you cannot readily associate behavior changes to adding or removing classes and even doing so may defy user expectations.

See the discussion in #676 for additional background.

👩‍💻 Implementation

The following API changes were made per component:

breadcrumb

  • [appearance] enum has keys: default, prominent
    • removed class .prominent-links

button

  • [appearance-variant] enum has keys default, primary
    • removed class .primary

combobox

  • [error-visible] boolean
    • removed class .invalid

icon

  • [severity] enum has keys default, success, information, warning, error
    • removed classes .fail, .warning, .pass, .information

number-field

  • [error-visible] boolean
    • removed class .invalid

text-field

  • [full-bleed] boolean
    • removed class .clear-inline-padding
  • [error-visible] boolean
    • removed class .invalid

tooltip

  • [severity] enum has keys default, information, error
    • removed classes .fail, .information
  • [icon-visible] boolean attribute
    • removed class .icon-visible

In addition some minor refactors are included in this PR:

  • nimble-components
    • IHasErrorText renamed to ErrorPattern to match our other pattern conventions and used consistently
    • ButtonPattern was also updated to align
    • DesignSystem.tagFor used consistently in component templates instead of context.tagFor or hard-coded names
    • Various storybook updates such as generating uniqueIDs for tooltip stories, splitting text-customization story to be per component instead of a monolithic test, cleaned up shared storybook states
  • nimble-angular
    • Removed the login component of the example app to align with blazor app
  • nimble-blazor
    • Consistent whitespace formatting for razor templates

🧪 Testing

For nimble-components largely relied on chromatic and storybook for regressions. For components, angular, and blazor added the expected tests for new enumerated attributes (type tests, directive tests, and blazor component render tests).

✅ Checklist

@rajsite rajsite marked this pull request as ready for review September 21, 2022 17:48
@rajsite rajsite requested a review from mollykreis September 21, 2022 17:49
Co-authored-by: mollykreis <20542556+mollykreis@users.noreply.github.com>
@msmithNI
Copy link
Contributor

Heads up that the "Testing" section of your PR description is partially missing/ cut off - I only see "For nimble-components largely relied" in it.

@rajsite rajsite enabled auto-merge (squash) September 29, 2022 19:43
@rajsite rajsite merged commit 9023d71 into main Sep 29, 2022
@rajsite rajsite deleted the attribute-vs-classes branch September 29, 2022 19:53
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.

5 participants