-
Notifications
You must be signed in to change notification settings - Fork 158
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(fade-in-out): introducing new components #4608
feat(fade-in-out): introducing new components #4608
Conversation
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.
Good to see the progress @IgnacioBecerra!
entry.target.style.transitionTimingFunction = motion( | ||
'entrance', | ||
'expressive' | ||
); | ||
entry.target.style.transitionDuration = delay; | ||
entry.target.style.opacity = 1; | ||
if (!iterations) { | ||
observer.unobserve(entry.target); | ||
} | ||
} else { | ||
entry.target.style.transitionTimingFunction = motion( | ||
'exit', | ||
'expressive' | ||
); | ||
entry.target.style.transitionDuration = delay; | ||
entry.target.style.opacity = 0; |
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.
Would you be able to add/remove a CSS class, instead of applying inline style?
Deploy preview created for package Built with commit: 9b49564b5491d5bc9a4d5984bed9d4ec7be945ea |
Deploy preview created for package Built with commit: 9b49564b5491d5bc9a4d5984bed9d4ec7be945ea |
packages/utilities/src/utilities/scrollIntoView/scrollIntoView.js
Outdated
Show resolved
Hide resolved
|
||
const observer = new IntersectionObserver(function handleIntersect( | ||
entries | ||
) { | ||
root.style.setProperty('--delay', delay); |
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.
Probably we can simply let application set the CSS custom property in the application CSS?
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 func specs detailed an option to allow adopters to provide their custom delay, this is in case the adopter wants to use different delays for different components (e.g. target boxA
for 3s
and boxB
for 1s
or so).
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 see and the custom delay can be done by:
.app--some-area-containing-scroll-into-view {
--dds-delay: 3s
}
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.
Ah I see! That's definitely a better way to allow the adopter to have their own delay within their own CSS file.
packages/utilities/src/utilities/scrollIntoView/scroll-into-view.scss
Outdated
Show resolved
Hide resolved
packages/utilities/src/utilities/scrollIntoView/scroll-into-view.scss
Outdated
Show resolved
Hide resolved
packages/utilities/src/utilities/scrollIntoView/scroll-into-view.scss
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.
Thank you for the updates @IgnacioBecerra!
packages/utilities/src/utilities/scrollIntoView/scrollIntoView.js
Outdated
Show resolved
Hide resolved
packages/utilities/src/utilities/scrollIntoView/scrollIntoView.js
Outdated
Show resolved
Hide resolved
packages/utilities/src/utilities/scrollIntoView/scrollIntoView.js
Outdated
Show resolved
Hide resolved
@import '../../globals/imports'; | ||
|
||
:root { | ||
--#{$dds-prefix}--delay: 400ms; |
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.
My apologies for another name change request, but I figured that the variable should be more specific of what it does (instead of using name implying a general purpose):
--#{$dds-prefix}--delay: 400ms; | |
--#{$dds-prefix}--scroll-into-view-delay: 400ms; |
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're right, the more specific the better to avoid any potential conflicts 👍
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 👍 - Thanks @IgnacioBecerra!
Deploy preview created for package Built with commit: b367d52d60b99e97efbb652989696858abdc1e91 |
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.
Adding notes here from what we discussed offline:
- Create a story in the Dotcom Shell with the utility enabled, so we can see it in action
- The JSDocs generation isn't quite right, from my notes:
- it doesn't look like
scrollIntoView
is showing up in the JSDoc generation, butviewportMargin
is: https://ibmdotcom-utilities.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/4608/global.html#viewportMargin. you might need to makeviewportMargin
andoptions
into private variables (add@private
to the JSDoc description of those), and move the JSDoc declaration to on top ofscrollIntoView
. Also, renameviewportMargin
andoptions
to_viewportMargin
and_options
- it doesn't look like
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 @IgnacioBecerra for the update!
packages/web-components/src/components/fade-in-out/fade-in-out.ts
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.
Thanks @IgnacioBecerra! The last naming nit - While we use _
prefix for private/protected properties/methods, we don't use it for closure variables even if we meant it as private. And please make sure the CI passes. Almost there!
packages/web-components/src/components/fade-in-out/fade-in-out.ts
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.
LGTM 👍 - Let's see our designers' input - Thanks @IgnacioBecerra!
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.
Thank you for all of the updates @IgnacioBecerra ! As mentioned in slack, now that FadeInOut
is its own component now, it makes sense to have it as its own storybook story, along with Docs tab on how adopters can use.
@IgnacioBecerra looking good so far! For React, I don't think the knobs are necessary since they are related to the dotcom shell and not fade specifically. We can probably remove those for the fade storybook stories. EDIT: same for web components |
@@ -0,0 +1,24 @@ | |||
/** |
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.
Not sure if this file technically belongs in internal
, maybe move to components
since it is part of a component now. Be sure to update corresponding documentation too.
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.
Looking good! 👍
@IgnacioBecerra looks good! before I approve can you check on the CI checks? Probably need to update snapshot files. |
@jeffchew Yup, ran into a merge conflict with the new image update, just pushed a fix for that. Let's hope the CI passes |
@IgnacioBecerra not critical, but looks like the dotcom shell default in web components has a missing headline now: |
) ### Related Ticket(s) carbon-design-system#4281 ### Description Utility made for adopters that will enable them to include fade in/out animations for the ibm.com web pages they build. `fadeInOut` also includes parameter of `keepAnimation`: to define whether the animation persists after the first activation (defaults to `true`) `fadeInOut` also supports custom delay duration by setting `--#{$dds-prefix}--scroll-into-view-delay` to the desired delay inside the targeted class declaration. https://user-images.githubusercontent.com/24970122/104062914-2c0bd900-51b0-11eb-92df-b81717d1ccc4.mov ``` import '@carbon/ibmdotcom-styles/scss/internal/scroll-into-view/_scroll-into-view.scss'; const selectorTargets = 'bx--content-block, bx--content-group' // React import { FadeInOut } from '@carbon/ibmdotcom-react'; <FadeInOut selectorTargets={selectorTargets} keepAnimations={false} /> // Web Component import '@carbon/ibmdotcom-web-components/es/components/fade-in-out/fade-in-out.js'; <dds-fade-in-out .selectorTargets="${selectorTargets}"></dds-fade-in-out> // custom-app.scss .bx--content-block { --#{$dds-prefix}--fade-in-out-delay: 3s; // custom delay } ``` ### Storybook Variation Added a new Storybook variation for `Dotcom Shell`, `With Fade Animations` for testing purposes. Under `Other` knobs, one can adjust the delay in seconds to give an idea of the custom delay that can be used. ### Changelog **New** - `fade-in-out` React and Web Component introduced - `With Fade Animations` Storybook variations for `Dotcom Shell` for testing purposes <!-- React and Web Component deploy previews are enabled by default. --> <!-- To enable additional available deploy previews, apply the following --> <!-- labels for the corresponding package: --> <!-- *** "package: services": Services --> <!-- *** "package: utilities": Utilities --> <!-- *** "package: styles": Carbon Expressive --> <!-- *** "RTL": React / Web Components (RTL) --> <!-- *** "feature flag": React / Web Components (experimental) -->
Related Ticket(s)
#4281
Description
Utility made for adopters that will enable them to include fade in/out animations for the ibm.com web pages they build.
fadeInOut
also includes parameter ofkeepAnimation
: to define whether the animation persists after the first activation (defaults totrue
)fadeInOut
also supports custom delay duration by setting--#{$dds-prefix}--scroll-into-view-delay
to the desired delay inside the targeted class declaration.Screen.Recording.2021-01-08.at.12.50.42.PM.mov
Storybook Variation
Added a new Storybook variation for
Dotcom Shell
,With Fade Animations
for testing purposes.Under
Other
knobs, one can adjust the delay in seconds to give an idea of the custom delay that can be used.Changelog
New
fade-in-out
React and Web Component introducedWith Fade Animations
Storybook variations forDotcom Shell
for testing purposes