-
Notifications
You must be signed in to change notification settings - Fork 201
feat: Support secondaryText and labelTag for button-dropdown #3844
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
base: main
Are you sure you want to change the base?
feat: Support secondaryText and labelTag for button-dropdown #3844
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3844 +/- ##
=======================================
Coverage 97.16% 97.16%
=======================================
Files 850 850
Lines 24785 24791 +6
Branches 8735 8737 +2
=======================================
+ Hits 24082 24088 +6
Misses 696 696
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
There are different changes in the file, I only expect to see your changes though. Could you please merge from main again and update the snapshot? I believe there was a change regarding generating the documenter and that is why this has different changes.
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.
Merged the changes from main but still see the gaps in documenter snap.
It seems some description/text has been reorders, could be related to the generation logic change you mentioned
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.
Pull the latest main and rebuild the documenter.test.ts.snap file.
…description-deprecation
src/button-dropdown/interfaces.ts
Outdated
* - `secondaryText` (string) - (Optional) Secondary text displayed in the menu for this item. | ||
* - `lang` (string) - (Optional) The language of the item, provided as a BCP 47 language tag. | ||
* - `disabled` (boolean) - whether the item is disabled. Disabled items are not clickable, but they can be highlighted with the keyboard to make them accessible. | ||
* - `disabledReason` (string) - (Optional) Displays text near the `text` property when item is disabled. Use to provide additional context. | ||
* - `description` (string) - additional data that will be passed to a `data-description` attribute. | ||
* - `description` (string) - additional data that will be passed to a `data-description` attribute. **Deprecated**, has no effect. | ||
* - `ariaLabel` (string) - (Optional) - ARIA label of the item element. | ||
* - `labelTag` (string) - (Optional) - Label for the item. Displayed near the item text for visually impaired users. |
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.
The new APIs shouldn't be added here because this list is all the APIs available in all types which is not the case for the new APIs, they are only available in action and checkbox so instead of here, they should be down
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.
Moved these 2 API description below to applicable types.
.text-with-icon { | ||
display: flex; | ||
align-items: center; | ||
} |
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.
This is not used anywhere?
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.
Removed in latest PR
Rebased from the latest origin main and resolve the conflict. |
/** | ||
* Finds the label tag element for a dropdown item. Returns null if the element has no label tag. | ||
*/ | ||
findLabelTags(): ElementWrapper | null { |
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.
findLabelTag
is more correct
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.
Updated to findLabelTag
<InternalBox fontWeight="light" data-testid={'button-dropdown-label-tag'}> | ||
{item.labelTag} | ||
</InternalBox> |
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.
Instead of having data-testid, I suggest wrapping {item.labelTag}
with a span and a className. For className to work, you'll need to have the className defined in the styles files with a comment like this:
.label-tag {
/* used in test-utils */
}
Additionally, something we started doing recently is having a different styles file where we add the classNames used for test-util so here you can follow a similar approach where you create a folder called test-classes
under button-dropdown folder and create a new styles.scss
where you have this class defined. The same thing applies to the secondaryText below. You don't need to define any other class in that - the old ones should stay as is.
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.
Or actually, instead of using InternalBox, use a div with a classname and apply the styles to the div. For color you can use this design token color-text-dropdown-item-secondary
, this way the users who wish to change the colors will be able to do so. With InternalBox a different design token would be used which is an implementation detail and users wouldn't expect that design token to be used here.
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.
And one more thing, the label tag shouldn't have the same color with description. In select component, it is that way and it's better to keep it consistent.
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.
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.
* - `secondaryText` (string) - (Optional) Secondary text displayed in the menu for this item. | ||
* - `labelTag` (string) - (Optional) - Label for the item. Displayed near the item text for visually impaired users. |
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.
In the documenter snapshot these still appear in their old place, could you update the snapshot again?
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.
Pull and build again and now new props are showing on the right place
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.
The integ tests are failing since these pages are updated but the tests not. Could you run them locally and update as expected?
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.
Can you suggest how to resolve the following issue when run npm run test:integ
?
FatalError: Jest: Got error running globalSetup - /Volumes/workplace/cloudscape-components/components/build-tools/integ/global-setup.js, reason: Cannot find local chromedriver. Did you run 'npm i -g chromedriver'?
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.
You need to follow these instructions: https://github.com/cloudscape-design/components/blob/main/CONTRIBUTING.md#run-integration-tests.
Basically, you globally install chromedriver
and then run the integ tests. You can specifically run the button dropdown tests only using this command:
npx jest -c jest.integ.config.js src/button-dropdown/__integ__
Make sure that you have the dev server (npm start
) running somewhere else in the background
"path": "lib/components/internal/widget-exports.js", | ||
"brotli": false, | ||
"limit": "962 kB", | ||
"limit": "963 kB", |
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.
are we sure this is needed?
href: 'https://instances.com', | ||
external: true, | ||
externalIconAriaLabel: '(opens in new tab)', | ||
labelTag: 'Ctrl + Click', |
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.
Something broke here. If you go to this permutations page in the browser, you'll see the following error message:
hook.js:608 Warning: React does not recognize the
labelTag
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercaselabeltag
instead. If you accidentally passed it from a parent component, remove it from the DOM element.
That's also why the a11y tests are failing
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.
There's a few integ tests that are failing. This is because you've added the new descriptions in a lot of places, which is now breaking the element text assertions.
Please 1) make sure to update test assertions where needed, and 2) review if we really need to add the new descriptions to all 9 button-dropdown pages that you're currently touching. I suggest we keep it simpler and only add it to the permutation pages and maybe 1-2 integ test pags.
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.
You need to follow these instructions: https://github.com/cloudscape-design/components/blob/main/CONTRIBUTING.md#run-integration-tests.
Basically, you globally install chromedriver
and then run the integ tests. You can specifically run the button dropdown tests only using this command:
npx jest -c jest.integ.config.js src/button-dropdown/__integ__
Make sure that you have the dev server (npm start
) running somewhere else in the background
<InternalBox fontWeight="light" data-testid={'button-dropdown-label-tag'}> | ||
{item.labelTag} | ||
</InternalBox> |
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.
This reverts commit f43c798.
…description-deprecation
Description
Added support for secondaryText and labelTag properties to ButtonDropdown items, enabling richer item content similar to other Cloudscape components like Select and Multiselect.
This PR also added deprecated annotation for
description
prop for button dropdown item.New features:
secondaryText: Displays additional descriptive text below the main item text
labelTag: Adds a visual tag/label to items for categorization or status indication
Implementation details:
deprecated
annotation into thedescription
propfindSecondaryText
andfindLabelTag
test utilsRelated links, issue #, if available: n/a
How has this been tested?
Unit Tests:
Manual Testing:
Reviewers can test by running:
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.