-
Notifications
You must be signed in to change notification settings - Fork 25
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/consistency with button animations #2351
base: master
Are you sure you want to change the base?
Conversation
… to see what I should do instead
…uired. Will commit regardless and see what team feedback has to say. Opted for this because there is an ion button on the tile and the tile itself is not such a button.
Making PR.
src/app/shared/components/template/components/tile-component/tile-component.component.scss
Outdated
Show resolved
Hide resolved
src/app/shared/components/template/components/button/button.component.html
Outdated
Show resolved
Hide resolved
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.
Apologies, I've just returned to review this PR. See my comments inline.
Additionally, there seems to be a discrepancy between the ripple effect applied to the button card
variant and the card-portrait
variant. The former is smooth and slow, but the latter appears fast and glitchy (possibly two ripple effects being applied?). See video below for a demo, see if you can figure out how to resolve this.
ripple.mov
Also I think it might be best to revert the changes made to the tile component so that this PR just applies to the card-portrait
button variant. Then you could update the PR title and description accordingly.
@@ -25,16 +25,18 @@ | |||
[attr.data-rowname]="_row.name" | |||
></plh-template-component> | |||
</span> | |||
<ion-ripple-effect></ion-ripple-effect> |
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 card
variant already had a ripple effect, so this can be removed as it is redundant
@@ -47,6 +49,7 @@ | |||
[parent]="parent" | |||
[attr.data-rowname]="_row.name" | |||
></plh-template-component> | |||
<ion-ripple-effect></ion-ripple-effect> |
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 can be removed – the button itself already has a ripple effect applied by line 39
Improved by changing the duration of all ripple effects by using Screen.Recording.2024-08-28.at.15.29.56.movDue to the dimensions of the buttons, I think it may create an idea that it is moving faster than the other. From reading the doc, the ripple effect works by trying to move across the button to its dimensions. It seems to aim to do it by an unknown time, but it depends on how wide and long the button is. |
PR Checklist
Description
Adds the ripple effect to most buttons specified.
This ensures that consistency is maintained.
Git Issues
Closes #2232
Screenshots/Videos
Evidence is for button types I am currently aware of:
Button card (no changes were made)
Screen.Recording.2024-07-09.at.16.09.59.mov
Button card portrait
Screen.Recording.2024-07-09.at.16.11.27.mov
Disabled parameter buttons
Screen.Recording.2024-07-09.at.16.12.26.mov
Button colour buttons
Screen.Recording.2024-07-09.at.16.13.48.mov
Button width
Screen.Recording.2024-07-09.at.16.14.39.mov
Button height
Screen.Recording.2024-07-09.at.16.29.35.mov
Button text colour
Screen.Recording.2024-07-09.at.16.31.04.mov
The rest were omitted because they were under the subheading: "To be removed"