upcoming: [DI-20928] - Introduce label for all global filters #11118
Conversation
…while showing filter
agorthi-akamai
force-pushed
the
feature/global_filter_titles
branch
from
fe554f4 to
c3ea831
Compare
label changes
…kamai/manager into feature/global_filter_titles
venkymano-akamai
requested review from
jdamore-linode
and removed request for
a team
There was a problem hiding this comment.
I'm seeing all the labels for the filters and not seeing any regressions to the core components that were modified in styles or props, so this was looking good to me.
Test coverage is passing - there was one CI e2e failure on a clone linode spec that has been consistently flaky and is unrelated to these changes.
I had left one comment about copy: "DB" vs "Database".
| name: 'DB Clusters', | ||
| neededInServicePage: false, | ||
| placeholder: 'Select a DB Cluster', | ||
| placeholder: 'Select DB Clusters', |
There was a problem hiding this comment.
Minor, non-blocking:
How did UX feel about using DB in the copy here? (cc @gitkcosby) I couldn't find another place in Cloud Manager where we use "DB" rather than spell out "Database".
| name: 'DB Clusters', | |
| neededInServicePage: false, | |
| placeholder: 'Select a DB Cluster', | |
| placeholder: 'Select DB Clusters', | |
| name: 'Database Clusters', | |
| neededInServicePage: false, | |
| placeholder: 'Select Database Clusters', |
There was a problem hiding this comment.
@mjac0bs , Yes upon discussion with our tech writer, we have moved from DB to Database as suggested. Also we have update the Engine filter to Database Engine and its respective placeholder to Select a Database Engine
|
Coverage Report: ✅ |
| @@ -172,6 +173,7 @@ export const RegionSelect = < | |||
| label={label ?? 'Region'} | |||
| loading={accountAvailabilityLoading} | |||
| loadingText="Loading regions..." | |||
| noMarginTop={noMarginTop ?? false} | |||
There was a problem hiding this comment.
No need to put the ?? false. This component can accept undefined
| noMarginTop={noMarginTop ?? false} | |
| noMarginTop={noMarginTop} |
| PopperComponent={(props: PopperProps) => ( | ||
| <CustomPopper {...props} placement="bottom" /> | ||
| )} |
There was a problem hiding this comment.
Rather than using CustomPopper directly here, think we can use MUI's slotProps to pass the prop to the underlying popper.
| PopperComponent={(props: PopperProps) => ( | |
| <CustomPopper {...props} placement="bottom" /> | |
| )} | |
| slotProps={{ | |
| popper: { | |
| placement: "bottom" | |
| }, | |
| }} |
|
@bnussman-akamai / @mjac0bs , thanks for the approvals, i have addressed the minor comments from both of you. since we have two approvals, it is ready for merging. |
Cloud Manager E2E
|
||||||||||||||||||||||||||||
| Project |
Cloud Manager E2E
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 28m 02s |
| Commit |
|
| Committer | venkatmano-akamai |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
6
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
438
|
| View all changes introduced in this branch ↗︎ | |
Description 📝
Added label for all ACLP Global filters using filter config
Changes 🔄
Target release date 🗓️
17-10-2024
Preview 📷
How to test 🧪
As an Author I have considered 🤔
Check all that apply