-
Notifications
You must be signed in to change notification settings - Fork 308
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
BREAKING: Removing stylistic data-attributes #4452
BREAKING: Removing stylistic data-attributes #4452
Conversation
🦋 Changeset detectedLatest commit: 401b240 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -43,7 +41,6 @@ exports[`ChangePassword renders as expected 1`] = ` | |||
aria-checked="false" | |||
aria-label="Show password" | |||
class="amplify-button amplify-field-group__control amplify-field__show-password" | |||
data-fullwidth="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.
This isn't used either? I don't see a corresponding classname above
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.
Correct, doing a search in the repo for data-fullwidth
only comes up with snapshot tests and vue/angular code. No CSS references it.
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.
Approved, assuming my comment above is not relevant
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! Left a couple clarifying questions about data
attributes that are remaining, and saw that some unit test names reference the data
attributes that no longer exist (would be good to audit the related unit test names in general, didn't comment on all of them)
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>
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.
PR feedback handling 10 out of 10, would review 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.
Thanks for cleaning this up!
Description of changes
These were a tech debt from a while ago cleaning it up.
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addedsideEffects
field updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.