-
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
Changes from 9 commits
caea15c
e0827fd
d7c4f8b
370161c
7ac3c4e
b3bf936
f2f28e0
d697a49
f2f6b40
65cd0cc
348cff7
e55580c
879fe94
a9d195d
6e07260
610ce46
0b34be4
7c23d01
ed05f92
461edc1
8264ddf
40bbba9
077b0dd
e5eedd1
35eb572
d62d6d4
5392a08
90a961a
41fbdd5
95f4af5
72c1a97
a9a4511
0425ce4
54c0f5b
2cffb6f
a82d128
794ec81
67e7e32
93768f9
671764b
8df2faf
4671464
972fbc2
7cacbe7
84b6fe4
afa297e
1749f5c
20cc29f
3f4450a
8379ff1
39bc570
e7c06ad
e46286c
43ce5aa
b4291d5
79e7913
86772a5
b38bd30
a4683a2
6a22d06
beb0771
0eb0cd1
c643950
b367d52
3e1acb0
a364613
6604d52
399aa63
9b49564
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,25 @@ | ||||||
/** | ||||||
* Copyright IBM Corp. 2016, 2020 | ||||||
* | ||||||
* This source code is licensed under the Apache-2.0 license found in the | ||||||
* LICENSE file in the root directory of this source tree. | ||||||
*/ | ||||||
|
||||||
@import '../../globals/vars'; | ||||||
@import '../../globals/imports'; | ||||||
|
||||||
:root { | ||||||
--#{$dds-prefix}--scroll-into-view-delay: 400ms; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure we're using Carbon's tokens.
Suggested change
|
||||||
} | ||||||
|
||||||
.bx--fade-in { | ||||||
transition-timing-function: carbon--motion(entrance, expressive); | ||||||
transition-duration: var(--#{$dds-prefix}--scroll-into-view-delay); | ||||||
opacity: 1; | ||||||
} | ||||||
|
||||||
.bx--fade-out { | ||||||
transition-timing-function: carbon--motion(exit, expressive); | ||||||
transition-duration: var(--#{$dds-prefix}--scroll-into-view-delay); | ||||||
opacity: 0; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/** | ||
* Copyright IBM Corp. 2016, 2020 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
export { default as scrollIntoView } from './scrollIntoView'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/** | ||
* Copyright IBM Corp. 2016, 2020 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/** | ||
* Utility handles fade transition for selected elements. | ||
* | ||
* @example | ||
* import '@carbon/ibmdotcom-styles/scss/internal/scroll-into-view/_scroll-into-view.scss'; | ||
* import { scrollIntoView } from '@carbon/ibmdotcom-utilities'; | ||
* | ||
* As an example, the function can be called to target 'bx--content-block' as such: | ||
* | ||
* For default values of 400ms and continuous play: | ||
* scrollIntoView('.bx--content-block') | ||
* | ||
* With 'one and done' option: | ||
* scrollIntoView('.bx--content-block', false) | ||
* | ||
* For custom delay time, set within targeted class in the application's CSS code as such: | ||
* | ||
* .bx--content-block { | ||
* --#{$dds-prefix}--scroll-into-view-delay: 3s; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API here feels a little off to me. You have everything defined by the utility function above. Could the utility set the property? // adopter
scrollIntoView('.bx--content-block', 3s);
// utiliity
function scrollIntoView(selector, delay = 0, iterations = false) {
...
selector.style.setProperty('--dds--scroll-into-view-delay', delay);
...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm yeah, I originally wrote it like this, but since we're trying to move away from inline styling, I moved forward with the other approach by having the adopters set the delay within their own classes. I think both approaches have their pros and cons, but I do agree it'd be easier for the adopter to set it through the API. @asudoh I'm wondering if you have some thoughts about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @IgnacioBecerra is right, we are avoiding defining style by JavaScript unless it's absolutely necessary. If we use inline style there is no way for application style to override it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asudoh not sure I'm fully following that logic. What's a scenario that is "absolutely necessary"? The adopters would be the ones setting this, wouldn't they? This isn't stopping them from resetting it, does it? It seems odd to me they'd have to touch two separate files to get this working, and it uses two different sets of APIs. It also feels laborious to implement a proper delay that builds on top of itself using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @photodow Application may want to define the rule of scrolling delay enforced to everywhere in the application, or everywhere in a specific portion. However, if a JavaScript code defines inline style, such inline style wins all the time and thus there is no way (that is natural in CSS world) for setting it application wide. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asudoh yeah, I'm very familiar with how CSS specificity works in this scenario. I'm not bought into the user experience you're creating here at the moment. I also don't know if we want other parts of the CSS overriding this value. I personally feel like this would be a scenario where inline would make sense, but for the sake of progress I'll step aside, and we can see where this takes us. Thanks as always @asudoh! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @photodow for your thoughts! Another option is using an enum (that corresponds to a set of CSS classes) rather than a direct number. The story-specific CSS rulesets will be moved to our styles package in this way. |
||
* } | ||
* | ||
* @param {*} selector menu item selector id | ||
* @param {boolean} iterations to define whether its continuous or not | ||
*/ | ||
|
||
const viewportMargin = | ||
'-' + | ||
( | ||
Math.round(document.documentElement.clientHeight / (100 / 12.5)) + 32 | ||
).toString() + | ||
'px 0px'; | ||
|
||
const options = { | ||
rootMargin: viewportMargin, | ||
threshold: 0, | ||
}; | ||
|
||
const scrollIntoView = (selector, iterations = true) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we set iterations default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mere question, but will this only allow for a string selector, or can it pass in a Node object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently it's set to receive strings, mainly since it's wrapped around a listener that waits until the whole page is loaded before querying the element. If the adopters can make sure the element exists before calling the function we could make it so they can pass node objects instead. |
||
window.addEventListener( | ||
'load', | ||
() => { | ||
const elements = document.querySelectorAll(selector); | ||
const observer = new IntersectionObserver(function handleIntersect( | ||
entries | ||
) { | ||
entries.forEach(entry => { | ||
if (entry.intersectionRatio > 0) { | ||
entry.target.classList.remove('bx--fade-out'); | ||
entry.target.classList.add('bx--fade-in'); | ||
if (!iterations) { | ||
observer.unobserve(entry.target); | ||
} | ||
} else { | ||
entry.target.classList.remove('bx--fade-in'); | ||
entry.target.classList.add('bx--fade-out'); | ||
} | ||
}); | ||
}, | ||
options); | ||
|
||
elements.forEach(e => { | ||
observer.observe(e); | ||
}); | ||
}, | ||
false | ||
); | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I now see one thing that slipped my mind... My apologies that I didn't notice thing earlier. We need to detach event listeners, A quick way to solve this is adding something like below: return {
release() {
_resizeObserver.disconnect();
_innerObserver.disconnect();
_rootObserver.disconnect();
window.removeEventListener('load', handleOnLoad);
},
}; Another way (that I'd prefer given scroll into view thing now effectively has a lifecycle as in
CC @jeffchew There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For performance purposes a component wrapper might make more sense for this. Perhaps we can build web component wrapper first, and would we be able to create the react wrapper of that or a pure react component would make more sense @asudoh @IgnacioBecerra ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jeffchew Web Component and a React wrapper on top of it makes sense to me - @IgnacioBecerra what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jeffchew @asudoh Hm, just so I understand, would creating a Web Component for this would be something like:
After typing it out like this I think I understand it better, but let me know if my understanding is wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question @IgnacioBecerra! We have two choices:
Basically, if we tend to (or can) add fade in/out effect to "region"s or components rather than some indivisual (specific) elements, 1. works the best (e.g. are they only heading/copy elements that need fade in/out effect in content block?). Otherwise 2. works better. |
||
|
||
export default scrollIntoView; |
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 tocomponents
since it is part of a component now. Be sure to update corresponding documentation too.