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

(web-components) Cleaned up shared styles for button and input for improved reuse and color updates #24929

Merged
merged 4 commits into from
Oct 20, 2022

Conversation

bheston
Copy link
Contributor

@bheston bheston commented Sep 23, 2022

Current Behavior

Currently a few components share some common styling, but it's organized such that some attributes must be overwritten or removed if a different component appearance doesn't need them. This has made applying some color recipe updates that support opacity for use of mica difficult.

New Behavior

Following #24771

This PR cleans up the shared styles to they are more composable and removes the need to reverse attributes that were unnecessarily set.

This update does not produce a visual difference aside from Combobox which currently is missing the focus underline treatment it should have. This PR fixes that, aside from that there are no other visual changes I found.

Anchor
Anchor update

Anchor HC
Anchor HC update

Button
Buttons update

Button HC
Buttons HC update

Inputs
Inputs update

Inputs HC
Inputs HC update

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 23, 2022

📊 Bundle size report

🤖 This report was generated against 3757c89720f0273ed00441f11104a000fe0edc52

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 23, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f58e0ba:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 23, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 3757c89720f0273ed00441f11104a000fe0edc52 (build)

Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

I don't think we should rename all the exports as part of a style fix. Also, the renaming pattern doesn't match how we have handled internals in other packages.

_lightweightButtonStyles,
_neutralButtonStyles,
_outlineButtonStyles,
_stealthButtonStyles,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is strange since we don't have this naming convention anywhere else in any of our codebases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. I was under the impression that internals weren't considered part of the public API and I was just resolving the warning the API report generates. I've reverted this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you were wrong in doing that. It's more of what we have as a patter. I think we could talk about doing this more broadly in the future.

@bheston bheston force-pushed the shared-styles-cleanup branch from 6b1e1cc to f58e0ba Compare October 20, 2022 21:48
@chrisdholt chrisdholt merged commit b63fc36 into microsoft:master Oct 20, 2022
@bheston bheston deleted the shared-styles-cleanup branch October 28, 2022 18:53
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
…proved reuse and color updates (microsoft#24929)

* Cleaned up shared styles for button and input for improved reuse and color updates

* Change file

* Remove underscore update on internals.

* Fix lint issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants