-
Notifications
You must be signed in to change notification settings - Fork 11
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
Reorganize and rename components #171
Reorganize and rename components #171
Conversation
…ockLinkCarousel, simplifying template for MixinCarousel, and moving slide rendering to respective components
…ckLinkCard and BlockLinkTile
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.
@stephiescastle great work with the reorg and renaming of components! I tested the build and looked over the code and it all looks good. As far as your question about the Cards
, I don't really have much of an opinion on that other than I don't want to create more work for you; but if you found it helpful seeing them grouped together then perhaps others will also find it more helpful with them separated.
Hmm, this is tricky. I do think it's helpful to see these three things grouped together somewhere, but are "tiles" in fact a variation of a card? If not, I think it's potentially confusing to group them under "Card types" or a new Components > Cards section. If they are a variation of a card, then perhaps |
That migration table is 🤌 |
Maybe not a variant, but they are a type of "thing that can be interchangeable with other things in a list context." We never landed on a general term for things like that, one of which could be "card." I'm hesitant to use something like Hmm.. will continue to think on it. |
Good points, @stephiescastle. I think I like sticking with what you currently have, just grouping them on the Blocks overview page. |
Discussed the SemVer tag with @Scotchester, and we decided to keep this PR slated for a minor release, not major. Specifically: leave the old class names in place with a to-be-deprecated notice. Example: in I'll work on making these changes before merging. |
@Scotchester I've pushed a commit that keeps this backwards compatible and updated the migration notes to reflect that. I'll also keep the "cards" as is on the Blocks overview page. I believe this is ready for another review. |
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.
Looks great, thank you!
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.
all looks/sounds good to me :) Thanks, Stephanie!!
Checklist
Description
This PR addresses #130 and #147.
AnimationCaret
renamed toMixinAnimationCaret
BaseVideoBg
renamed toMixinVideoBg
BaseCarouselCards
renamed toMixinCarousel
ArticleCarousel
to a more genericBlockLinkCarousel
, which now has anitemType
argument to choose between:ArticleCarouselItem
renamed toBlockLinkCard
TopicsCarouselItem
newly added (Add TopicsCarousel component #147) and renamed toBlockLinkTile
BlockFactCards
toBlockCardGroup
BlockFactCardsItem
toBlockCard
HeroFeature
renamed toHeroMedium
and referred to as "Medium"HeroFocalPoint
renamed toHeroLarge
and referred to as "Large"HeroMedia
referred to as "Media Only"Noted follow-up
compact
options are not yet available in Explorer 1 forBlockLinkCard
andBlockLinkTile
(fkaArticleCarouselItem
andTopicsCarouselItem
). These will be handled, among other improvements, in Update ArticleCarouselItem and TopicsCarouselItem to includecompact
versions,headingLevel
prop, and improved label #58Questions
Should cards move to a separate section?
I created a "Card types" heading on the Blocks Overview page and listed
BlockCard
,BlockLinkCard
, andBlockLinkTile
there. I debated moving them to their own components section, perhaps: Components > Cards, but wanted to get some input before doing so.I do think it's helpful to see them grouped together, but another option would be to have
BlockLinkCard
andBlockLinkTile
live underBlockLinkCarousel
, andBlockCard
to live underBlockCardGroup
. I liked bringing them more to the foreground however, as they can be reused in different contexts, but I guess none of our components really support swapping them ad lib just yet, except where it's already documented (BlockLinkCarousel
).Migration Notes
Various components have been renamed, including their corresponding SCSS and JavaScript includes. You should check your projects for any of the following and update accordingly. Note that all new names are documented in the table below, but not all new names require action.
_MixinCarousel.scss
/src/js/vendors/_swiper.js
and/src/js/vendors/_swiperOptions.js
. If importing exports directly from_swiperOptions
, note that the export name has changed toMixinCarousel
_MixinAnimationCaret.scss
_HeroMedium.scss
_HeroLarge.scss
Old component names will be supported until the next major release.
New features added to
BlockLinkCarousel
Additionally,
BlockLinkCarousel
has a new "item type" argument.BlockLinkCard
, the default selection, replicates what was formerly known asBlockArticleCarousel
. A new option,BlockLinkTile
, is now available as well.MixinCarousel
improvementThe
-tile
variant ofMixinCarousel
now includespb-5
by default. When used in projects, the-tile
variant required adding thepb-5
inline class to work properly with tiles. If this applies to your project, you can now remove thepb-5
inline class from the following selectors:.BaseCarouselCards.-tile
or.MixinCarousel.-tile
Recommended
If you are using Explorer 1 with a custom frontend framework, you may want to consider renaming your components, and/or creating a table documenting how your project's components map to the Explorer 1 design system.
Instructions to test
BlockLinkCard
andBlockLinkTile
inBlockLinkCarousel
, it will only work in canvas view, not docs view. This is a known issue: DocsPage controls do not work for iframe stories #145Tested in the following environments/browsers:
Operating System
Browser