-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-2193 Add bubble splash effect that can be added to various buttons #2013
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #2013 +/- ##
=======================================
Coverage 73.64% 73.64%
=======================================
Files 428 428
Lines 24271 24271
Branches 3888 3888
=======================================
Hits 17875 17875
Misses 5803 5803
Partials 593 593 ☔ View full report in Codecov by Sentry. |
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.
Overall looks good. A couple comments.
Curious if you considered using a third-party library to simplify the creation of this? See e.g. the feature/confetti branch from a few months ago.
Was it intentional that the size of bubbles seems to be scaled relative to button size? I found that a bit odd. I thought if it was necessary to scale the size of the buttons, that would make sense as a parameter.
I am not taking responsibility for reviewing this, just swinging by to add a few comments.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @siltomato)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.directive.ts
line 11 at r2 (raw file):
@Directive({ selector: '[sfBubbleButton]' })
Does Angular not allow specifying CSS for a directive?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.stories.ts
line 26 at r2 (raw file):
type Story = StoryObj; export const MatRaisedButton: Story = {
There are a lot of stories here that all do the same thing in just about the same way (so far as I can tell). Chromatic bills us per screenshot. We're not going to go broke due to having six more screenshots than necessary, but if we consistently create several times as many as really necessary, we're likely to put ourselves in a position where we're forced to upgrade to a plan with more screenshots.
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.stories.ts
line 28 at r2 (raw file):
export const MatRaisedButton: Story = { render: () => ({ template: `<button mat-raised-button sfBubbleButton color="primary">Mat raised button</button>`
This is a lot more verbose than necessary on all these stories. Instead of specifying a render function on all of them, it seems like it would make more sense to use args. Maybe that wouldn't work as well as I think in this situation.
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.
I like it. I noticed that the short buttons only have a little bit of celebration. I think we should decide on the amount of celebration (for example, the size of the bubbles, and how far out they fly), and make it consistent for large and small buttons.
I think it does make sense to use a 3rd party library. It seems like something that someone else would have already provided. I also haven't looked into it. I also like the bubbles. :)
I think I'm learning here rather than in a position to provide a scrutinous review. But I considered the files and made comments.
Thanks for your work!
Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Nateowami and @siltomato)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 6 at r2 (raw file):
/// Gets the min/max css function to constrain the size within a min and max size in px. /// @param {number} $sizePercentage - The size of the bubble as a percentage of the button size.
Yeah, maybe this could be based on something other than the size of the button, so it's uniform across buttons of varying lengths. Maybe it could be based on
- height of the button,
- height or width of the web page's viewport, or
- a constant px or em value, if that looks good on different viewports
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 111 at r2 (raw file):
right: -20%; // Needed to make the padding increase the height as the width increases.
I wonder if it will result in UI elements getting pushed around, if the button is in the midst of other UI elements when this happens?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 114 at r2 (raw file):
// Long buttons need more height for the animation so the circles don't get cut off. box-sizing: content-box; padding-top: 30%; // Percentage of width of containing element
Do you mean "height" instead of "width"?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 114 at r2 (raw file):
// Long buttons need more height for the animation so the circles don't get cut off. box-sizing: content-box; padding-top: 30%; // Percentage of width of containing element
Because the bubbles fly out at known range of pixels, wouldn't you want to set padding-top to close to the value of the maximum px distance that a bubble might fly out? Or what is the purpose of using a percentage of the size of the containing element?
Also, would you not also need a padding-bottom?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 121 at r2 (raw file):
&:before { display: none;
Why is display
configured both here and above in the following? Is that a mistake, or is there a particular behaviour that causes?
&:before,
&:after {
position: absolute;
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.scss
line 129 at r2 (raw file):
&:after { display: none; top: 65%;
(And this is not at odds with the following? )
&:before,
&:after {
...
height: 100%;
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.stories.ts
line 26 at r2 (raw file):
Previously, Nateowami (Nathaniel Paulus) wrote…
There are a lot of stories here that all do the same thing in just about the same way (so far as I can tell). Chromatic bills us per screenshot. We're not going to go broke due to having six more screenshots than necessary, but if we consistently create several times as many as really necessary, we're likely to put ourselves in a position where we're forced to upgrade to a plan with more screenshots.
@Nateowami, :-/ Can we disconnect some of our storybook tests from Chromatic?
I found it helpful to click thru the different UI cases and see the behaviour for them. It will be too bad if using Chromatic resulted in needing to restrict ourselves.
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.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @marksvc and @siltomato)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.stories.ts
line 26 at r2 (raw file):
Previously, marksvc wrote…
@Nateowami, :-/ Can we disconnect some of our storybook tests from Chromatic?
I found it helpful to click thru the different UI cases and see the behaviour for them. It will be too bad if using Chromatic resulted in needing to restrict ourselves.
Yes, that can be done. Though I thought args would make more sense, and allow even more flexibility with experimenting with possible variations.
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.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @siltomato)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/bubble-button/bubble-button.stories.ts
line 26 at r2 (raw file):
Previously, Nateowami (Nathaniel Paulus) wrote…
Yes, that can be done. Though I thought args would make more sense, and allow even more flexibility with experimenting with possible variations.
Actually, come to think of it, Chromatic isn't going to be able to do a good job screenshotting an animation anyway, so my point was kind of moot. @siltomato Can you just disable Chromatic for this entire file? https://www.chromatic.com/docs/ignoring-elements
Prototype bubble splash effect to add to various buttons as an angular directive. Part of gamification initiative to add visual rewards for app actions. The directive can be added to angular material buttons. Currently, the angular directive is only in Storybook for preview and feedback.
Sample buttons can be seen in Storybook under 'Misc/Bubble Button'.
Note:
sass-loader
,css-loader
, andstyle-loader
to be able to load the directive stylesheet in its story.This change is