-
Notifications
You must be signed in to change notification settings - Fork 162
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
feat: Property filter token groups #2626
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2626 +/- ##
==========================================
+ Coverage 95.87% 95.89% +0.02%
==========================================
Files 748 748
Lines 20730 20719 -11
Branches 7006 7056 +50
==========================================
- Hits 19874 19869 -5
- Misses 800 842 +42
+ Partials 56 8 -48 ☔ View full report in Codecov by Sentry. |
19eac3c
to
f3a4b50
Compare
0413ab2
to
233f369
Compare
@@ -14,6 +14,8 @@ $token-grouped-padding-block: 2px; | |||
$token-grouped-padding-inline: styles.$control-padding-horizontal; | |||
$inner-token-padding-block: 1px; | |||
$inner-token-padding-inline: styles.$control-padding-horizontal; | |||
$border-radius-token: awsui.$border-radius-token; |
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.
Why do we redeclare it 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.
Just for consistency with border-radius-inner-token
border-start-start-radius: $border-radius-inner-token; | ||
border-start-end-radius: $border-radius-inner-token; | ||
border-end-start-radius: $border-radius-inner-token; | ||
border-end-end-radius: $border-radius-inner-token; |
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.
Why do we do it this way and not just use border-radius: $border-radius-inner-token;
?
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 border-radius is banned by stylelint because of RTL.
@@ -42,12 +52,18 @@ const PropertyFilter = React.forwardRef( | |||
}, | |||
}; | |||
|
|||
if (hideOperations && enableTokenGroups) { | |||
warnOnce('PropertyFilter', 'Operations cannot be hidden when token groups are enabled.'); | |||
hideOperations = false; |
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.
We don't normally edit values that have been provided to us as it can be confusing for the developer that they provided one value but the component then does not respect that value. Is a warning not enough 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.
That is not really editing as there is no mutation involved. By assigning this to false we ensure the value is ignored. We do use a similar approach here: https://github.com/cloudscape-design/components/blob/main/src/date-range-picker/index.tsx#L237
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.
A few general questions.
0f61b2e
to
413a948
Compare
Description
Making property filter token groups API public. The PR also includes small changes to default popover position, alignment and border radius of nested tokens, dev-warning for hide-tokens.
Depends on:
Related changes:
Rel: [HJhfABYOa8OE]
How has this been tested?
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.