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: Consolidating dropdown/NavDropdown user experience (removing React-bootstrap, using AntD) #14557
fix: Consolidating dropdown/NavDropdown user experience (removing React-bootstrap, using AntD) #14557
Conversation
/testenv up |
@rusackas Container image not yet published for this PR. Please try again when build is complete. |
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details. |
thanks for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just need to fix those broken tests
@michael-s-molina please check up on the failing unit tests when you're able. Appreciate this PR though - so close to the end of the tunnel! |
c3d7169
to
c6a7d05
Compare
Done. |
Codecov Report
@@ Coverage Diff @@
## master #14557 +/- ##
=======================================
Coverage 77.38% 77.39%
=======================================
Files 959 958 -1
Lines 48465 48461 -4
Branches 5678 5676 -2
=======================================
- Hits 37506 37504 -2
+ Misses 10759 10757 -2
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Done. |
/testenv up |
@junlincc Ephemeral environment spinning up at http://18.237.254.24:8080. Credentials are |
Sorry to be a stickler, but wondering about two things, based just on the screenshot:
|
c6a7d05
to
458c3ee
Compare
Both done. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://54.218.47.33:8080. Credentials are |
</NavDropdown> | ||
listHeight={400} | ||
dropdownAlign={{ | ||
offset: [-135, 0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also do some styling to reposition this, e.g.:
transform: translateX(-100%);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rusackas I changed to make the component wrap big texts in multiple lines instead of using transform
to avoid fat dropdowns 😉. Since the width is fixed, the offset is always correctly positioned. If you agree, you can remove the need:followup
label.
Let's get that CSS transform added in separate PR. I don't see it as blocking for now. |
458c3ee
to
69286ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the width/offset change! LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
NavDropdown
from Bootstrap to AntD.Menu
toSelect
.See: #10254
@rusackas @junlincc @pkdotson
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
1 - Enable multiple languages feature flag
2 - Check language picker at the top right corner
ADDITIONAL INFORMATION