ref(poc): composition #36703
ref(poc): composition #36703
Conversation
github-actions
bot
added
the
Scope: Frontend
markstory
left a comment
markstory
left a comment
There was a problem hiding this comment.
Any reason you don't want to merge this? It looks like a positive change to me.
|
@markstory I havent really verified that it works exactly as before (that margin 0 is missing for instance), was mostly meant to demonstrate what I meant |
| justify-content: flex-start; | ||
| :hover ${StyledIconAnchor} { | ||
| &:hover ${StyledIconAnchor} { |
There was a problem hiding this comment.
please, what is the difference between &:hover and :hover?
There was a problem hiding this comment.
& is a stand in for the parent. It's specifically less/sass, it will compile to something like
.Perma-c1273 {
display: inline-flex;
justify-content: flex-start;
...
}
.Perma-c1273:hover {
display: block;
...
}I hadn't thought much about it before, but that it works with just :hover in emotion is kind of weird, it's probably some emotion-specific shorthand, which imo for one character we probably shouldn't be doing.
tldr; I agree with Jonas replacing it here.
There was a problem hiding this comment.
@k-fish I actually replaced it thinking that it is broken and that it probably doesn't compile correctly 😅
There was a problem hiding this comment.
oh, I see... can we enforce this through eslint, so we can have everywhere the same pattern? and thanks for the explanation @k-fish 🙏
|
I like the idea too, but before we can merge it, we have to make sure that everything is according to the previous styles. Great PoC @JonasBa! Thanks for sharing this 👏 |
k-fish
left a comment
k-fish
left a comment
There was a problem hiding this comment.
Nice 👍, I agree about shipping it as an example (if it's not too much work to fix styles)
| interface PermalinkTitleProps | ||
| extends React.DetailedHTMLProps< | ||
| AnchorHTMLAttributes<HTMLAnchorElement>, | ||
| HTMLAnchorElement | ||
| > {} | ||
|
|
||
| export function PermalinkTitle(props: PermalinkTitleProps) { | ||
| return ( | ||
| <Permalink href={'#' + props.type} className="permalink" {...props}> | ||
| <StyledIconAnchor /> | ||
| {t('Stack Trace')} | ||
| </Permalink> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Yeah, nice example Jonas 👏 . I usually call this specialization as per https://reactjs.org/docs/composition-vs-inheritance.html, I've heard it called it a number of names in both eng side and design (used to call it 'molecules' in an atom-based design system).
Definitely one of the patterns I reach for the most since you can have the ease-of-use of a simple api / use for common "combinations" of more generic underlying components.
There was a problem hiding this comment.
Ah yes 💯 you explained this perfectly, I was not really sure how to call it. I might have read that and just forgot about it
| justify-content: flex-start; | ||
| :hover ${StyledIconAnchor} { | ||
| &:hover ${StyledIconAnchor} { |
There was a problem hiding this comment.
& is a stand in for the parent. It's specifically less/sass, it will compile to something like
.Perma-c1273 {
display: inline-flex;
justify-content: flex-start;
...
}
.Perma-c1273:hover {
display: block;
...
}I hadn't thought much about it before, but that it works with just :hover in emotion is kind of weird, it's probably some emotion-specific shorthand, which imo for one character we probably shouldn't be doing.
tldr; I agree with Jonas replacing it here.
|
@priscilawebdev let me know if you would have 10min to spare to pair on this PR and wrap it up. It's a bit hard for me to assert everything works as intended. |
|
@JonasBa is this still a draft PR? |
|
@priscilawebdev nope, I hadn't noticed I left it as a draft. It should be good to go, I'd appreciate if you can give it another look to make sure I haven't missed anything |
priscilawebdev
left a comment
priscilawebdev
left a comment
There was a problem hiding this comment.
checked locally and it looks good to me
| stackTraceNotFound: boolean; | ||
| stackType: STACK_TYPE; | ||
| title: React.ReactNode; | ||
| title: React.ReactElement<any, any>; |
There was a problem hiding this comment.
did we change this because it can be null or undefined?
There was a problem hiding this comment.
I changed it because you cant cloneElement on null/undefined and it didnt look like null/undefined is intended usage.
This is a proof of concept PR that is not meant to be merged, I created it as a showcase on how leveraging composition can help reduce API surface and increase flexibility of the codebase.
The changes in the PR are the following
With the changes introduced here, you can see that we have removed one prop from the component while simultaneously also exposed the api of PermalinkTitle which was previously inaccessible to the consumer, meaning we can now independently pass custom props to each instance of the PermalinkTitle component without having to extend the API of our TraceEvent component and avoid prop drilling entirely.
To summarize the functional benefits:
In an ideal scenario, the PermalinkTitle would have been a core UI library component so as to avoid creating new custom components around the codebase and the Title component would not have included any margins by default so that we would not have to override them.
Some resources: