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

fix: Replacing bottom border styles with text decoration underline in Link #24734

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

khmakoto
Copy link
Member

@khmakoto khmakoto commented Sep 8, 2022

Current Behavior

Link uses border-bottom styles for its underline.

New Behavior

Link uses text-decoration: underline styles for its underline, which is more inline with most implementations out there.

Related Issue(s)

Fixes #24729

@khmakoto khmakoto enabled auto-merge (squash) September 8, 2022 23:42
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 8, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-link
Link
12.254 kB
4.956 kB
11.784 kB
4.867 kB
-470 B
-89 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
187.656 kB
51.96 kB
react-components
react-components: FluentProvider & webLightTheme
33.359 kB
11.004 kB
react-portal-compat
PortalCompatProvider
5.851 kB
1.964 kB
🤖 This report was generated against 7f8bad094dadcdb671d5746848e96b5bd4b5791a

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 8, 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 19ffa30:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
throbbing-sun-upbxd3 Issue #24729

@size-auditor
Copy link

size-auditor bot commented Sep 8, 2022

Asset size changes

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

Baseline commit: 7f8bad094dadcdb671d5746848e96b5bd4b5791a (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 8, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1619 1647 5000
Button mount 1168 1149 5000
FluentProvider mount 1969 1944 5000
FluentProviderWithTheme mount 726 720 10
FluentProviderWithTheme virtual-rerender 670 674 10
FluentProviderWithTheme virtual-rerender-with-unmount 713 728 10
MakeStyles mount 2318 2358 50000
SpinButton mount 3230 3229 5000

Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

textDecorationColor defaults to currentcolor, and it doesn't look like you ever set it to anything other than the same as the color right? (When the underline should be visible.)

I'd propose replacing all of the places that set textDecorationColor: tokens.color..., to instead set textDecorationLine: 'underline'. And anywhere that sets textDecorationColor: 'transparent' to instead set textDecorationLine: 'none'.

This should marginally improve perf (fewer css variable lookups). It would also reduce bundle size (fewer griffel classes since all places that use textDecorationLine: 'underline' would share the same class). It would also let you get rid of the inlineSubtle class, since you could just use inline in all cases. And it might even avoid a potential bug in the future if someone changes the text color without realizing they need to change the underline color too.

I'll add code suggestions in a few places to clarify.

…orationLine for improved bundle size and performance.
@khmakoto khmakoto merged commit f97dc9f into microsoft:master Sep 12, 2022
@khmakoto khmakoto deleted the linkTextDecoration branch September 12, 2022 18:10
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 14, 2022
* master: (28 commits)
  Fix value font-weight inside heatmap chart (microsoft#24726)
  Fix legend overflow-indication-text role (microsoft#24756)
  Support custom locale in date axis  (microsoft#24753)
  Cleanup env variables (microsoft#24739)
  ci(github): add GH Action to add issue labels based on new GH issue template (microsoft#24788)
  Update disallowedChangeTypes for newly created packages, to allow only 'prerelease' change types by default (microsoft#24763)
  feat(react-components): Adding missing AvatarGroup exports (microsoft#24770)
  remove unnecessary nohoist (microsoft#24760)
  feat(react-dialog): supports 1st rule of ARIA (microsoft#24525)
  BREAKING: TableCell layouts are handled by layout components (microsoft#24762)
  feat: Implement table cell layout components (microsoft#24773)
  applying package updates
  fix: remove readonly from DetailsList (microsoft#24615)
  chore: Cleaning up tokens in Button components so they better adhere to the design spec (microsoft#24732)
  fix: react-combobox listbox popup width matches trigger width (microsoft#24733)
  fix: react-combobox Option focus outline only shows with keyboard nav (microsoft#24700)
  feat: Publish react-field package, and export from react-components/unstable (microsoft#24235)
  fix: Replacing bottom border styles with text decoration underline in Link (microsoft#24734)
  docs(react-theme): Update readme (microsoft#24755)
  Add tests for hover states (microsoft#24390)
  ...
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.

[react-link]: use textDecoration instead of border as underline
4 participants