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

feat(icon-switch): design updates #2819

Merged
merged 16 commits into from
Sep 22, 2021
Merged

Conversation

kevinsperrine
Copy link
Collaborator

@kevinsperrine kevinsperrine commented Sep 9, 2021

Closes #1536

Summary

  • Bring IconSwitch into alignment with new styles.

Icon content switcher - Style.pdf

Change List (commits, features, bugs, etc)

  • update styles
  • add disabled/children props to IconSwitch
  • update stories to include more icons and show the divider
  • add new stories

Acceptance Test (how to verify the PR)

  • confirm styles match those shown in the PDF above

Regression Test (how to make sure this PR doesn't break old functionality)

  • confirm this doesn't break interactions or designs in other stories that use the switchers.
  • DashboardEditor
  • ImageGalleryModal
  • HotspotTextStyleTab

Things to look for during review

  • Make sure all references to iot or bx class prefix is using the prefix variable
  • (React) All major areas have a data-testid attribute. New test ids should have test written to ensure they are not changed or removed.
  • UI should be checked in RTL mode to ensure the proper handling of layout and text.
  • All strings should be translatable.
  • The code should pass a11y scans (The storybook a11y knob should show no violations). New components should have a11y test written.
  • Unit test should be written and should have a coverage of 90% or higher in all areas.
  • All components should be passing visual regression test. For new styles or components either a visual regression test should be written for all permutations or the base image updated.
  • Changes or new components should either write new or update existing documentation.
  • PR should link and close out an existing issue

@netlify
Copy link

netlify bot commented Sep 9, 2021

✔️ Deploy Preview for ai-apps-pal-angular ready!

🔨 Explore the source changes: 3073310

🔍 Inspect the deploy log: https://app.netlify.com/sites/ai-apps-pal-angular/deploys/6148b8ea20897200070dc341

😎 Browse the preview: https://deploy-preview-2819--ai-apps-pal-angular.netlify.app

@netlify
Copy link

netlify bot commented Sep 9, 2021

❌ Deploy Preview for carbon-addons-iot-react failed.

🔨 Explore the source changes: 3073310

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-addons-iot-react/deploys/6148b8ea4dc35a0008fa881f

@kevinsperrine kevinsperrine marked this pull request as ready for review September 10, 2021 16:00
@bjornalm
Copy link
Contributor

bjornalm commented Sep 13, 2021

When I select the different buttons the icons inside move slightly.
click-2-573cb55b7f0e

Kevin:
In light mode, this one gets tricky. The borders don't exist on the L/R rides until the item is selected. Additionally, because the border exists on the top bottom and has to exist on each icon instead of a parent given the nature of how these icons can be used within or outside of the ContentSwitcher, the borders on the L/R can't be width of 1px and transparent for example or you'll get a 1px gap at the top/bottom where the solid border meets the transparent border. :-\ I've experimented with box-shadow on the L/R but it's rendered differently than a border and interferes with the outline when focused. I suppose I can use the icon-switch__divider div that I added and add another pseudo-element to it, but that's the only solution I can think of unless you have another idea.

Bjorn:
The gap and the icon-switch__divider might be possible to solve it with border-image and gradients..
https://jsfiddle.net/hz8wp0L0/

if not, perhaps the outmost border around all the buttons can be on a container instead for on the buttons?

@bjornalm
Copy link
Contributor

bjornalm commented Sep 13, 2021

The corners look a bit weird here in the dark button.
image
It is in RTL mode for https://deploy-preview-2819--carbon-addons-iot-react.netlify.app/?path=/story/1-watson-iot-imagegallerymodal--basic

Same story and setup, possibly related, the selection also looks strange:
image

@kevinsperrine
Copy link
Collaborator Author

@bjornalm Should all be covered now.

@bjornalm
Copy link
Contributor

Are the latest changes in the preview deployment? I'm still seeing some strange things.

Selection in RTL still looks off:
image

Hovering shows the divider:
image

The selection is only shown on top and bottom
image

Perhaps we can look at how Carbon solved it for the content switcher? In a smaller container/window it looks like our iconSwitch
https://react.carbondesignsystem.com/?path=/story/components-contentswitcher--default&knob-Selection%20mode%20(selectionMode)=automatic&knob-[Deprecated]:%20Light%20variant%20(light)=true

@kevinsperrine
Copy link
Collaborator Author

Are the latest changes in the preview deployment? I'm still seeing some strange things.

Selection in RTL still looks off:
image

Hovering shows the divider:
image

The selection is only shown on top and bottom
image

Perhaps we can look at how Carbon solved it for the content switcher? In a smaller container/window it looks like our iconSwitch
https://react.carbondesignsystem.com/?path=/story/components-contentswitcher--default&knob-Selection%20mode%20(selectionMode)=automatic&knob-[Deprecated]:%20Light%20variant%20(light)=true

Ugh. This design more complicated than it should be. fixed those issues. just waiting for them to push and deploy.

@davidicus
Copy link
Collaborator

@kevinsperrine Doesn't the spec call for rounded borders even when focused?

@kevinsperrine
Copy link
Collaborator Author

@kevinsperrine Doesn't the spec call for rounded borders even when focused?

Yeah, that was part of the problem. I had accidentally used 1px radius instead of the 0.25rem radius, so that's why it looked funky.

@davidicus
Copy link
Collaborator

davidicus commented Sep 16, 2021

Internal buttons outline looks a little light. I think I saw that you are overwriting the outline prop and doing something with gradients. Perhaps we can scope those to :not(first-child), :not(last-child)? I think this looks pretty good but thought I mention it.
image

@kevinsperrine
Copy link
Collaborator Author

Internal buttons outline looks a little light. I think I saw that you are overwriting the outline prop and doing something with gradients. Perhaps we can scope those to :not(first-child), :not(last-child)? I think this looks pretty good but thought I mention it.
image

By light, so you mean the top/bottom or left/right? I can thicken the L/R if that's what you mean...

@bjornalm
Copy link
Contributor

bjornalm commented Sep 20, 2021

This is looking much better!

One last small thing is that the divider is still showing between a selected or hovered button a normal state button. It is really nitty gritty but if we want this component to be pixel perfect we should probably address it. The divider makes the line look a bit "fuzzy":

image

and if we disable the component it becomes more visible and on both sides:

image

@kevinsperrine
Copy link
Collaborator Author

This is looking much better!

One last small thing is that the divider is still showing between a selected or hovered button a normal state button. It is really nitty gritty but if we want this component to be pixel perfect we should probably address it. The divider makes the line look a bit "fuzzy":

image

and if we disable the component it becomes more visible and on both sides:

image

Are we done yet? 😉 https://deploy-preview-2819--carbon-addons-iot-react.netlify.app/?path=/story/1-watson-iot-iconswitch--example-used-in-content-switcher

@bjornalm
Copy link
Contributor

This is looking much better!
One last small thing is that the divider is still showing between a selected or hovered button a normal state button. It is really nitty gritty but if we want this component to be pixel perfect we should probably address it. The divider makes the line look a bit "fuzzy":
image
and if we disable the component it becomes more visible and on both sides:
image

Are we done yet? 😉 https://deploy-preview-2819--carbon-addons-iot-react.netlify.app/?path=/story/1-watson-iot-iconswitch--example-used-in-content-switcher

Hehe, I have actually found more stuff, but I won't torture you with that. Nothing that will break a build.

Copy link
Contributor

@bjornalm bjornalm left a comment

Choose a reason for hiding this comment

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

Great work, you have earned a pixel-perfect badge!

display: none;
}

// hide the divider beside it on hover to prevent "fuzziness" of the hover state
Copy link
Contributor

Choose a reason for hiding this comment

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

hehe

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:-) There are so many random little rules in there, I figured it good to label some of them.

@kevinsperrine
Copy link
Collaborator Author

@davidicus This one is ready to merge now, but coveralls is down, so we'll either need to wait for that or force merge. https://status.coveralls.io/

@kodiakhq kodiakhq bot merged commit 13957d8 into next Sep 22, 2021
@kodiakhq kodiakhq bot deleted the 1536-icon-switch-style-updates branch September 22, 2021 01:22
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.

[Icon content switcher] Styling enhancement
3 participants