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

UI: Styling updates #14820

Merged
merged 9 commits into from
May 6, 2021
Merged

UI: Styling updates #14820

merged 9 commits into from
May 6, 2021

Conversation

domyen
Copy link
Member

@domyen domyen commented May 5, 2021

Issue: #14737 and other UI improvements

What I did

  • ArgsTable when in controls panel: Fix ugly black border in Safari
  • Boolean control: Switch position of True | False to False | True per Appearance of Boolean controls #14737
  • Sidebar: Buttons for root heading and expand/collapse now only get the outline-y focus state if they're keyboarded to
  • Toolbar icons: Update copy icon to a link (which is what it does) and share icon to the alternative share icon
  • Icons: Update icon set to have parity with the design system
  • a11y addon: minor layout alignment fixes

How to test

  • Is this testable with Jest or Chromatic screenshots? Yes. Go to the PR check.
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

@domyen domyen added the ui label May 5, 2021
@nx-cloud
Copy link

nx-cloud bot commented May 5, 2021

Nx Cloud Report

CI ran the following commands for commit 943f762. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@domyen domyen added the maintenance User-facing maintenance tasks label May 5, 2021
@shilman shilman changed the title Styling updates UI: Styling updates May 6, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looks great @domyen! 💯

@shilman shilman merged commit aa05079 into next May 6, 2021
@shilman shilman deleted the styling-updates branch May 6, 2021 02:43
@shilman
Copy link
Member

shilman commented May 6, 2021

@domyen I think this PR introduces a slight regression. In the case where there's a boolean arg with an undefined value, in the old version the control would display false. After this PR the control displays true. While neither option is technically correct, the old behavior is more correct than the new one. I think we are dealing with this in the "undefined values" work, so I'm not sure if it's critical to fix, but I wanted to get it on your radar.

@domyen
Copy link
Member Author

domyen commented May 6, 2021

@shilman That's strange, this is a styling change, it doesn't affect whether the boolean was checked. When I go to the Storybook itself I see that it still displays "false" here.

image

@ndelangen
Copy link
Member

🤔

@shilman
Copy link
Member

shilman commented May 7, 2021

@domyen I'll keep an eye out for it after the undefined values changes come in and provide a repro if it's still broken then

@domyen
Copy link
Member Author

domyen commented May 7, 2021

Thanks! I handed the undefined state project back to @tmeasday yesterday so he'll probably know if anything is wrong.

@shilman
Copy link
Member

shilman commented May 8, 2021

Ok @tmeasday @domyen you can see the changes in every new PR that has unaccepted changes related to this bug. I'm not sure why it didn't show up in the original PR.

Example: https://www.chromatic.com/test?appId=5a375b97f4b14f0020b0cda3&id=609673a78e8200003929f724

@shilman
Copy link
Member

shilman commented May 8, 2021

False alarm. It was introduced here: #14408, reverting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants