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(Tile): update outline-offset #8258

Merged
merged 10 commits into from
Apr 6, 2021
Merged

fix(Tile): update outline-offset #8258

merged 10 commits into from
Apr 6, 2021

Conversation

andreancardona
Copy link
Contributor

@andreancardona andreancardona commented Mar 31, 2021 •
edited
Loading

Closes #6306

  • fixes RadioTile outline bug that can be seen here & below:

Screen Shot 2021-03-31 at 2 44 40 PM

Testing / Reviewing

@andreancardona andreancardona requested a review from a team as a code owner March 31, 2021 20:47
@andreancardona andreancardona mentioned this pull request Mar 31, 2021
Closed
86 tasks
@andreancardona andreancardona changed the title 6306 radio tile outline bug fix(Tile): update outline-offset Mar 31, 2021
@andreancardona andreancardona mentioned this pull request Mar 31, 2021
Closed
@netlify
Copy link

netlify bot commented Mar 31, 2021 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit f58c2f0

https://deploy-preview-8258--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Mar 31, 2021 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit f58c2f0

https://deploy-preview-8258--carbon-components-react.netlify.app

@tw15egan
Copy link
Collaborator

The radio looks great!

Still seeing some cutoff on the multi select variant
Screen Shot 2021-03-31 at 5 13 34 PM

@andreancardona
Copy link
Contributor Author

The radio looks great!

Still seeing some cutoff on the multi select variant
Screen Shot 2021-03-31 at 5 13 34 PM

Ooo I see! Going to confirm a visual change. Converting this to a draft in the mean time

@andreancardona andreancardona marked this pull request as draft March 31, 2021 22:05
@andreancardona
Copy link
Contributor Author

andreancardona commented Mar 31, 2021 •
edited
Loading

@laurenmrice would like your opinion on the visual change to the black border to 2px for the following reasons:

  • It matches the focus state 2px outline
  • and fixes all the bugs/issues

Thank you!

@laurenmrice
Copy link
Member

@andreancardona The border for the selected state needs to still be 1px. We use 1px for borders like this across our system unless it is a focus state.

@andreancardona andreancardona marked this pull request as ready for review April 2, 2021 15:53
@andreancardona
Copy link
Contributor Author

@tw15egan should be ready for review!

jnm2377
jnm2377 approved these changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@jnm2377 jnm2377 requested a review from laurenmrice April 2, 2021 20:50
laurenmrice
laurenmrice requested changes Apr 2, 2021
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

  • When there are two selected tiles next to eachother, there should not be an overlap of the tile borders. It should still look like 1px border between the tiles.
    Artboard

  • When you select a tile, there is a slight jump of the tile text moving out of place. The text should always remain in the same place not matter what state.
    Apr-02-2021 16-00-08

@andreancardona
Copy link
Contributor Author

andreancardona commented Apr 2, 2021 •
edited
Loading

@laurenmrice I'll make sure to look into the jumping text (I think it's an outline v border issue)

But when there are 2 selected tiles - that is the current styling- is that something we still want to update?
Screen Shot 2021-04-02 at 5 40 59 PM

@laurenmrice
Copy link
Member

@andreancardona Yes, we want to fix that, that is a bug.

@andreancardona andreancardona mentioned this pull request Apr 5, 2021
Closed
tw15egan
tw15egan approved these changes Apr 5, 2021
View reviewed changes
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅ 🎉

laurenmrice
laurenmrice approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good ! 🙌

@kodiakhq kodiakhq bot merged commit fc3abf0 into carbon-design-system:main Apr 6, 2021
@tw15egan tw15egan mentioned this pull request Apr 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

@tw15egan tw15egan tw15egan approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Radio Tile Group outline disappear problem
4 participants
@andreancardona @tw15egan @laurenmrice @jnm2377