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(devtools): update menu list css #22660

Closed
wants to merge 3 commits into from
Closed

Conversation

jyash97
Copy link
Contributor

@jyash97 jyash97 commented Oct 30, 2021

Summary

In devtools when we select a deeply nested component, and try to select parent component using the dropdown. Text overlaps with the background text since the menu list is rendered outside the dom so CSS var is not applied properly.

Check below recording for the issue:

MenuListIssue

How did you test this change?

Tested using inline devtools testing example. Below is the screen recording for testing the CSS:
MenuListThemeFix

@jyash97
Copy link
Contributor Author

jyash97 commented Nov 1, 2021

Hey @bvaughn 👋

Can you take a look at this changes, whenever you get time ?

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

CI is failing with a lint error. Please run yarn prettier to fix.

@jyash97
Copy link
Contributor Author

jyash97 commented Nov 1, 2021

Many CI check seems to be failing now. Any idea?

Looks unrelated to the changes I did in latest commit

@bvaughn
Copy link
Contributor

bvaughn commented Nov 1, 2021

Can you try rebasing on main ?

git rebase main

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This is looking a lot better!

Now the only thing that's broken is the hover, I think. Probably need to do the same thing for the MenuItem CSS class as we did for the MenuList class.

It's really unfortunate how our CSS colors get forked between constants.js and root.css because of these global styles ☹️ I wonder if there's better solution here.


Edit I filed #22678 to follow up on my above comment

@jyash97
Copy link
Contributor Author

jyash97 commented Nov 2, 2021

Now the only thing that's broken is the hover, I think

Yes its left, should we add another variable in root.css for hover background ?

It's really unfortunate how our CSS colors get forked between constants.js and root.css because of these global styles ☹️ I wonder if there's better solution here.

Yes I was unsure whether the solution I proposed was good since it adds the same values from constants.js to root.css. I have shared my thoughts here on the same: #22678 (comment)

@bvaughn
Copy link
Contributor

bvaughn commented Nov 3, 2021

Love the suggestion on #22678.

@jyash97
Copy link
Contributor Author

jyash97 commented Nov 3, 2021

Glad it helped 😄

Do I need to close this PR ? If not, can you help me with this:

Should we add another variable in root.css for hover background ?

@bvaughn
Copy link
Contributor

bvaughn commented Nov 3, 2021

Should we add another variable in root.css for hover background ?

For the "fix" to work fully with this PR, yes. We would need to do this as well. Or we could land the other fix first and then this would not be necessary.

@jyash97
Copy link
Contributor Author

jyash97 commented Nov 7, 2021

Closing as it will be fixed in #22716

@jyash97 jyash97 closed this Nov 7, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Nov 7, 2021

Love it

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.

None yet

3 participants