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

#8068: removing unnecessary css override for the button dropdown caret #8162

Merged
merged 1 commit into from Apr 5, 2024

Conversation

fungairino
Copy link
Collaborator

@fungairino fungairino commented Apr 4, 2024

What does this PR do?

Discussion

Let me know if there are any other placesI should check these particular uses of the dropdown (seems like the Deployment modal uses it as well).

I checked these in the PageEditor and it seems like it uses a different set of css sourced from bootstrapWithoutRem.css

Demo

Screenshot 2024-04-04 at 8 06 47 PM Screenshot 2024-04-04 at 8 05 12 PM

Checklist

  • Add tests and/or storybook stories
  • Designate a primary reviewer:

this css was hiding it, and now it can once again be displayed
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.35%. Comparing base (56f30b0) to head (5f4a804).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8162      +/-   ##
==========================================
- Coverage   73.36%   73.35%   -0.01%     
==========================================
  Files        1311     1311              
  Lines       40671    40671              
  Branches     7566     7566              
==========================================
- Hits        29839    29836       -3     
- Misses      10832    10835       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@twschiller twschiller added this to the 1.8.12 milestone Apr 4, 2024
@twschiller twschiller changed the title removing unecessary css override for the button dropdown caret #8068: removing unnecessary css override for the button dropdown caret Apr 4, 2024
@fungairino fungairino marked this pull request as ready for review April 5, 2024 00:14
@twschiller twschiller requested review from grahamlangford, BLoe and BrandonPxBx and removed request for twschiller April 5, 2024 00:29
@twschiller
Copy link
Contributor

twschiller commented Apr 5, 2024

We might want to tweak the vertical alignment? Right now the top of the carat seems to be at center line. Perhaps the middle of the carat should be there? Default bootstrap (and our Page Editor), for example, seems to put it centered? https://getbootstrap.com/docs/4.0/components/dropdowns/

image

Or it might be a product of vertical alignment for the text being off

In any case, I'd be OK release this PR as-is

@BrandonPxBx - thoughts?

@twschiller twschiller added the user experience Improve the user experience (UX) label Apr 5, 2024
Copy link

github-actions bot commented Apr 5, 2024

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@grahamlangford
Copy link
Collaborator

Approved, but we should fix the alignment in a follow-on PR.

@fungairino
Copy link
Collaborator Author

The vertical alignment is slightly off. compared to the page editor. I'll see if I can adjust real quick

@fungairino
Copy link
Collaborator Author

Doesn't seem an easy fix since there's a lot of little differences that are not immediately evident. For example the border radius on focus also is not present.

For now I'll merge and we can revisit this component UX again in the future as needed.

@fungairino fungairino merged commit 033f11d into main Apr 5, 2024
24 of 25 checks passed
@fungairino fungairino deleted the caret-enabled-for-button-dropdown branch April 5, 2024 14:16
@BrandonPxBx
Copy link

BrandonPxBx commented Apr 5, 2024

On alignment: neither of the examples are visually aligned. They're both a pixel or 2 off in the other direction -- I'd move the Page Editor one down a pixel, and the Ext. Con. up two pixel if I were being super nit picky. Consistency is probably the most important thing, though.

I'm curious because the page editor one seems to be a button component, while the console version is just an SVG (which is ironically non-focusable). Should we consider using the button component in the consoles as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user experience Improve the user experience (UX)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field toggle dropdown signifier doesn't show in Console/Sidebar
4 participants