refactor(ContentHero): update and apply component usage #12639
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
review needed 👀
tooling 🔧
TylerAPfledderer
force-pushed
the
refactor/ContentHero-content-adjustments
branch
from
8d31fcb to
4479689
Compare
TylerAPfledderer
requested review from
wackerow,
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
|
Hey @nloureiro ! Would like a review from you on these updates.
|
There was a problem hiding this comment.
Great job @TylerAPfledderer looks good!
Noting the same issue you mentioned
Wondering if this is something we should try to tackle @nloureiro. @TylerAPfledderer if so, we might need to accept imageProps so that the consumer can tweak the image as necessary.
It's not clear to me how that would be possible if this hero is being applied in a layout component. Unless you would be thinking of setting up some conditional at the layout level to set overrides based on the image file path provided by front matter. Is that what you are thinking of? |
|
@TylerAPfledderer I was basically thinking about overriding the defaults to try to recreate the same styles we had before in the // Roadmap layout
<ContentHero heroImg={frontmatter.image}>
// Upgrade layout
<ContentHero heroImg={frontmatter.image} imageProps={{ objectFit: "cover", ... }}> |
Oh! I could look at this. |
wackerow
added
the
needs design approval 🧑🎨
|
@pettinarip Is this something that should hold things up from a dev perspective? cc: @nloureiro for an approval |
No, I'd say that if designers are ok its ready to be merged |
@nloureiro provided the above links again for visibility with your review! |
|
Thank you @TylerAPfledderer I'm not sure where to give feedback on the chromatic part. Here or on the chromatic side? But for the dev preview link, I found some adjustments needed. let me know if this makes sense |
@nloureiro sorry, here is an updated changeset with the snapshot I think if there are changes needed that are in the scope of the PR, to deny the snapshot and add a comment in Chromatic underneath the snapshot. I think that is more streamlined for you and isolate the discussions. The anchor bar beneath the hero is not showing as a part of the component. I think this and removing the "last updated" content are a different scope. |
…ontent-adjustments
ok. I agree. I can open an issue to be worked on a different PR What I'm afraid of is having a "Frankenstein" look in between PRs, where the new components are done but not adjusted in the specifics like content and extra inline style that might have. |
|
Updated the branch and removed the "More Content" anchor link at the bottom |
…ontent-adjustments
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
…ontent-adjustments
…ontent-adjustments
…ontent-adjustments
|
@nloureiro https://www.chromatic.com/review?appId=63b7ea99632763723c7f4d6b&number=12639&type=linked&view=activity Currently blocking; can merge once approved |
It looks good to me! |
Description
This PR add modifications to the
ContentHerocomponent and applies it to theUpgradelayout.descriptionsection of the hero can accept either a string or componentsUpgradelayout, a list of descriptions render with a "last updated" timestampImagecomponent has new styling with predefined dimensionsheroImgprop to accept either a string or the static image data object, depending on the hero component to be used.Additional Information
This relieves a bug in the roadmap merge page where the alignment of the image and content in not centered and the image is clipped.