Skip to content

Conversation

@lordvkrum
Copy link
Contributor

@lordvkrum lordvkrum commented Aug 14, 2024

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

  • I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
  • I have added JSDoc comments to my TypeScript definitions for improved documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added any necessary documentation (if appropriate).
  • I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Documentation content changes
  • TypeScript type definitions update
  • Other... Please describe:

Changes for mobile view

split header and 2-column grid
Screenshot 2024-08-14 at 08 38 00

filters and sort options as modals
Screenshot 2024-08-14 at 08 38 09
Screenshot 2024-08-14 at 08 38 16

pagination resize with window
Screenshot 2024-08-14 at 08 38 23

@lordvkrum lordvkrum requested a review from a team as a code owner August 14, 2024 11:36
@linear
Copy link

linear bot commented Aug 14, 2024

CI-3648 MVP [OS UI PLP]: Mobile Friendly Styles for Filters

Business Outcome:

  • As a SDK Consumer I want Mobile Friendly Styles OOTB so that I do less work

Definition of done:

  • CSS magic

@lordvkrum lordvkrum requested review from HHHindawy and Mudaafi August 14, 2024 12:23
Copy link
Contributor

@Mudaafi Mudaafi left a comment

Choose a reason for hiding this comment

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

Looking good! I left some comments, but the one on using timeout for css transitions, let's discuss in our sync later today

// Calculate windowSize on resize event
useEffect(() => {
const resize = () => {
const parentSize = Number(pagesRef.current?.parentElement?.clientWidth) || window.innerWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I'm clear, we're checking for the parentElement's width first if it exists, else we go with the window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is mainly for edge cases (domless scenarios like jest testing)

Copy link
Contributor

Choose a reason for hiding this comment

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

no I think this is a good default. Most times, customers will use a container to host both the entire PLP, so setting the width to the container first is better

Copy link
Contributor

@Mudaafi Mudaafi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making the changes ❤️ I'll ask the team if anyone wants to take a second look, if not we can merge some time next week

@Mudaafi Mudaafi requested a review from a team August 24, 2024 00:34
@Mudaafi Mudaafi merged commit 88e6431 into main Aug 29, 2024
@Mudaafi Mudaafi deleted the ci-3648-mvp-os-ui-plp-mobile-friendly-styles-for-filters branch August 29, 2024 17:20
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.

3 participants