Skip to content
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

BBD Fractal Components #505

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

BBD Fractal Components #505

wants to merge 22 commits into from

Conversation

kilometerlime
Copy link
Collaborator

Tasks:
Create new block components:

  • quicklinks
  • master brand nav
  • card
  • carousel

Create new button variants

  • quicklink
  • quicklink-more
  • navigation

Copy link
Contributor

@methodog methodog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @milesbigbluedoor
Good job getting your head around using our Fractal library, your new components look good!
A few issues:

Units

  • Label-tag
    • Please rename your new variant to just --project. Sorry, it seems knit-picky, but better to name it more generically for re-use in future, and better to rename now before you implement into Wordpress.
  • Navigation Button
    • Not needed as it's already part of SitNav component. Sorry this wasn't clear, but the new buttons we want, as new variants of the Fractal Button Unit are the prev/next buttons to be used in your Carousel component (shown under "Icons" in the designs). You can mostly copy them over from our SearchPagination component, but tweaked to match the Carousel design. Then re-use the new Button Units back in your Carousel component.

Blocks

  • Carousel
    • Looks good. Sorry for wasted work, but please get rid of Swiper, we're trying to keep Fractal as free of dependencies as possible, and can implement swipe gestures with much smaller vanilla JS later.
    • See notes above under Navigation Button fro re-using new prev/next button variants here.
  • MasterBrandNav
    • This component is meant to be a much simpler thing - just the top bar, called "Global Navigation" in the designs.
      For the main Nav (called "Primary Navigation" in the designs) you can re-use the SiteNav component already in Fractal (just with appropriate changes to the HTML where you implemenet it on your end, and perhaps some modifying CSS in your site).

eslint errors

  • we always get these, I'm surprised you have so few, but you can see them on your local terminal where you run the site (npm run dev).
    I wouldn't bother you with them, but if you could resolve them please, your branch will automatically build to our QA site.

@kilometerlime
Copy link
Collaborator Author

Hey @methodog thank you for the detailed feedback! I've actioned all these changes in the above commits. Please let me know if its all okay. I made the following changes:

  • renamed the label tag.
  • reworked the master brand nav block component to be the correct component from figma.
  • added the ability for buttons to have an icon via context.
  • reworked the navigation button variant to be carousel navigation buttons (utilizing the changes made in the previous bullet point).
  • remove the swiper.js carousel library and replaced it with a basic carousel implementation using vanilla js.
  • cleaned up any linting errors I had missed from my terminal.

@kilometerlime kilometerlime requested a review from methodog April 12, 2024 05:16
Copy link
Contributor

@methodog methodog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @milesbigbluedoor
Thanks for these changes, and the clear commit messages and helpful PR comments.
Just a few niggling bits left I think - please see my comments in the code below...

src/components/blocks/card/card.html Outdated Show resolved Hide resolved
src/components/blocks/card/card.config.js Show resolved Hide resolved
src/assets/styles/vam-style.scss Outdated Show resolved Hide resolved
src/components/blocks/carousel/swiper-bundle.css Outdated Show resolved Hide resolved
src/components/blocks/carousel/carousel.html Outdated Show resolved Hide resolved
src/components/blocks/carousel/carousel.html Outdated Show resolved Hide resolved
src/components/units/button/button.config.js Outdated Show resolved Hide resolved
src/components/units/button/button.config.js Show resolved Hide resolved
@kilometerlime
Copy link
Collaborator Author

Hey @methodog apologies for missing these cleanup items, these should all be resolved now

@kilometerlime kilometerlime requested a review from methodog April 16, 2024 02:23
Copy link
Contributor

@methodog methodog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @milesbigbluedoor, i've pushed some various changes, mainly accessibility fixes. Please check my commits over - you can refer to how I've added our default focus styles when revisiting the Quicklinks block.
My code comments about responsive img tag size params are important for site optimisation, and worth mastering here as you'll need to get them right in Wordpress.

src/components/blocks/card/card.html Outdated Show resolved Hide resolved
src/components/blocks/carousel/carousel.html Outdated Show resolved Hide resolved
src/components/blocks/quicklinks/_quicklinks-preview.scss Outdated Show resolved Hide resolved
src/components/blocks/quicklinks/quicklinks.config.js Outdated Show resolved Hide resolved
src/components/blocks/quicklinks/quicklinks.html Outdated Show resolved Hide resolved
src/components/units/button/_button.js Outdated Show resolved Hide resolved
src/components/units/button/_button.scss Outdated Show resolved Hide resolved
src/components/units/button/button.config.js Outdated Show resolved Hide resolved
@methodog
Copy link
Contributor

thanks @milesbigbluedoor 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants