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.

Sign up for GitHub

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

Jump to bottom

fix(navitem): Remove blue lines in safari #1106

Merged
merged 4 commits into from
Oct 8, 2021
Merged

fix(navitem): Remove blue lines in safari #1106

merged 4 commits into from
Oct 8, 2021

Conversation

clairek20
Copy link
Contributor

@clairek20 clairek20 commented Sep 27, 2021 •
edited
Loading

Pull request - Remove blue lines in safari

Proposed changes

  • set outline to 0

Testing instructions

  • checkout storybook in safari
  • go to shell component
  • select a dropdown and see there should no longer be a blue outline

@clairek20 clairek20 requested a review from a team as a code owner September 27, 2021 13:57
@clairek20 clairek20 requested review from lily-peng and removed request for a team September 27, 2021 13:57
@netlify
Copy link

netlify bot commented Sep 27, 2021 •
edited
Loading

✔️ Deploy Preview for ibm-security ready!

🔨 Explore the source changes: 15eb8bb

🔍 Inspect the deploy log: https://app.netlify.com/sites/ibm-security/deploys/6160111ab930040008717115

😎 Browse the preview: https://deploy-preview-1106--ibm-security.netlify.app

cameroncalder
cameroncalder suggested changes Sep 28, 2021
View reviewed changes
Copy link

@cameroncalder cameroncalder left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this! The blue lines are gone, but the nav items with a tag now have more height than the ones without. The nav item height should be the same with or without the beta tag

shell.mov

@clairek20
Copy link
Contributor Author

Thanks for addressing this! The blue lines are gone, but the nav items with a tag now have more height than the ones without. The nav item height should be the same with or without the beta tag

shell.mov

Hey @cameroncalder

I was confused as to how removing an outline would effect a hover height, so i had a look at this locally without my change and that hover issue is still an issue with the beta tag. Seems like a different issue that wasn't caused by this change. It's strange though as if i go to https://ibm-security.carbondesignsystem.com/?path=/story/patterns-shell--default it's fine there, it's just an issue locally and in the deploy preview. I even looked at another PR and the deploy preview just to double check and i'm seeing the same #1108.

Could we maybe get this merged anyway? I can raise an issue for that in the mean time to investigate.

Thanks a million, Claire.

@cameroncalder
Copy link

@clairek20 appreciate the clarifying comment! Yes, we can merge this one to address the blue outline, and please do open another issue to further investigate the hover size discrepancy

@clairek20 clairek20 force-pushed the remove_blue_lines_safari branch from 05dfb0d to a183954 Compare October 7, 2021 18:08
@clairek20 clairek20 force-pushed the remove_blue_lines_safari branch from 2cf3d87 to b3d13a5 Compare October 8, 2021 09:36
@kodiakhq kodiakhq bot merged commit 5cae584 into dev Oct 8, 2021
@kodiakhq kodiakhq bot deleted the remove_blue_lines_safari branch October 8, 2021 09:42
SimonFinney pushed a commit that referenced this pull request Oct 8, 2021
@semantic-release-bot
chore(release): 1.45.0-prerelease.11 [skip ci]
13343c5 
# [1.45.0-prerelease.11](v1.45.0-prerelease.10...v1.45.0-prerelease.11) (2021-10-08)

### Bug Fixes

* **navitem:** Remove blue lines in safari ([#1106](#1106)) ([5cae584](5cae584))
@SimonFinney
Copy link
Contributor

🎉 This PR is included in version 1.45.0-prerelease.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

SimonFinney pushed a commit that referenced this pull request Oct 13, 2021
@semantic-release-bot
chore(release): 1.46.0 [skip ci]
03ae9de 
# [1.46.0](v1.45.0...v1.46.0) (2021-10-13)

### Bug Fixes

* **decorator:** fix icon `fill` styles ([#1103](#1103)) ([dea56f9](dea56f9))
* **nav-item-link:** remove fixed height to prevent issues with multi-line text ([#1085](#1085)) ([0e0f542](0e0f542))
* **navitem:** Remove blue lines in safari ([#1106](#1106)) ([5cae584](5cae584))
* **storybook:** Update addon-storysource for renamed Layout Modules ([#1108](#1108)) ([8a3933a](8a3933a))
* **tearsheet-small:** update maximum size styles ([#968](#968)) ([d8bc055](d8bc055))
* **toolbar:** vertical icon alignment for multi-line labels ([#1089](#1089)) ([0183591](0183591))

### Features

* **decorator:** add onContextMenu to decorator ([#1102](#1102)) ([46f29da](46f29da))
* **deps:** add remaining Carbon dependencies ([#1101](#1101)) ([c5bd47b](c5bd47b))
* **focus-trap-react:** upgrade package and fix dependency ([#1111](#1111)) ([ffe9c8a](ffe9c8a))

### Reverts

* Revert "Revert "feature(ICA): Add optional information and iconButton prop (#1110)" (#1115)" (#1116) ([92bac13](92bac13)), closes [#1110](#1110) [#1115](#1115) [#1116](#1116)
* Revert "feature(ICA): Add optional information and iconButton prop (#1110)" (#1115) ([86ad1c7](86ad1c7)), closes [#1110](#1110) [#1115](#1115)
@SimonFinney
Copy link
Contributor

🎉 This PR is included in version 1.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameroncalder cameroncalder cameroncalder approved these changes

@lily-peng lily-peng Awaiting requested review from lily-peng

Assignees
No one assigned
Labels
released on @canary released status: ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clairek20 @cameroncalder @SimonFinney