refactor(CodeSnippet): use hooks clean up render logic #4636
Conversation
ghost
requested review from
|
Deploy preview for the-carbon-components ready! Built with commit da0cea6 https://deploy-preview-4636--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit da0cea6 https://deploy-preview-4636--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit da0cea6 |
Co-Authored-By: emyarod <emyarod@users.noreply.github.com>
|
Would love to take a look at this, wanted to leave a comment just to say I'd love to review this before it gets merged in 👀 |
…o code-snippet-refactor
…o code-snippet-refactor
|
Random, noticed we swapped from const [shouldShowButton, setShouldShowButton] = useState(false);
const ref = useRef(null);
useLayoutEffect(() => {
if (ref.current) {
const { height } = ref.current.getBoundingClientRect();
setShouldShowButton(height > 255);
}
}, [children]);
// ...
<pre ref={ref}>{children}</pre>
// ... |
|
@joshblack on first render the ref is null, since ref updates don’t trigger a re-render, the hook doesn’t get called again. #4636 (comment) |
|
@vpicone is that due to the fact that the ref was destructured? Didn’t notice it locally when trying it. I think when you access a ref in an effect with the current property it’ll work. |
|
@joshblack 🤦♂ fixed, ty. |
|
@emyarod it does! |
| }) { | ||
| const [expandedCode, setExpandedCode] = useState(false); | ||
| const [shouldShowMoreLessBtn, setShouldShowMoreLessBtn] = useState(false); | ||
| const { current: uid } = useRef(getUniqueId()); |
There was a problem hiding this comment.
One thing that I don't think we remember is that technically we should not have ids start with a number, I think it's fine in HTML5 but in general can cause issues in certain cases (or at least has been brought up that way). Would we want to bring over our useId hook or add a prefix to this that's been passed to id?
There was a problem hiding this comment.
@joshblack our uniqueId actually adds an id prefix by default, so starting with a number shouldn't be an issue. Do you think we should be specifying a more descriptive prefix? I'm a little gun shy about tweaking ID generation behavior at this point haha.
There was a problem hiding this comment.
Ah, didn't realize it was using the older file for unique ids. In general, we use https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/tools/setupGetInstanceId.js for newer code
There was a problem hiding this comment.
@joshblack oh missed that. I think this is a great opportunity to switch to a hook. I really highly doubt someone's depending on the ID of the inline code component, but do you think it'd be worth keeping the id prefix?
There was a problem hiding this comment.
It's not that an external user is leveraging it, just that folks have pointed out that ids generated with a number as the first character technically doesn't adhere to the HTML4 spec (I think this is addressed in HTML5). I think we could totally do id-{number}, we just have to be careful around collisions and the reason this came up with the newer approach is that it's easy to collide since they don't all share the same global id variable (which helps out a ton for tests). I think combining the local prefix plus the global id generation in a hook would be great and useId could help do that
There was a problem hiding this comment.
I opened #4676 since I feel like changing the id generation behavior might be a little beyond the scope. The hook we have in the hooks package forces a - that doesn't exist in the current uniqueId implementation so it'd break some snapshots probably.
There was a problem hiding this comment.
@vpicone are we or others using automatically generated ids in tests?
There was a problem hiding this comment.
@joshblack wasn’t that the impetus for that big slack thread? Someone was using a generated ID to select an element in testing.
There was a problem hiding this comment.
@vpicone that was when someone used an id they passed into the component, not a generated one.
There was a problem hiding this comment.
Oh!!! That makes way more sense.
Co-Authored-By: Josh Black <josh@josh.black>
No external changes. One of perhaps several components related to #4635
Changed:
componendDidMountandcomponentDidUpdatelifecyclesuidrather than just assigning to class instanceTesting: