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.

Sign up for GitHub

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

Jump to bottom

feat[FX-3173]: Modernize /categories page #8379

Merged
merged 20 commits into from Sep 24, 2021
Merged

feat[FX-3173]: Modernize /categories page #8379

merged 20 commits into from Sep 24, 2021

Conversation

mdole
Copy link
Contributor

@mdole mdole commented Sep 9, 2021 •
edited

Included in this PR:

  • Establish new /categories v2 route
  • Deprecate old /categories route
  • Remove ScrollSpy dependency
  • Smoketest for /categories

Before:

Big/zoomed out browser widthScreen Shot 2021-09-21 at 7 28 11 PM
Small screenScreen Shot 2021-09-21 at 7 28 15 PM

After:

Big/zoomed out window, top Screen Shot 2021-09-23 at 2 38 47 PM
Big/zoomed out window, scrolled + stuckScreen Shot 2021-09-23 at 2 38 51 PM
Small screen, topScreen Shot 2021-09-23 at 2 39 00 PM
Small screen, scrolled + stuckScreen Shot 2021-09-23 at 2 39 04 PM
Jira ticket: https://artsyproduct.atlassian.net/browse/FX-3173

@mdole mdole self-assigned this Sep 9, 2021
@mdole mdole force-pushed the mdole/FX-3173/categories-palette-v3 branch 2 times, most recently from a349e5e to 98c4e9c Compare September 17, 2021 15:57
damassi
damassi reviewed Sep 17, 2021
View reviewed changes
src/v2/Apps/Categories/Components/StickyNav.tsx
return (
<SwiperRail {...props} display="block" position="relative" left={left} />
)
}
Copy link
Member

Choose a reason for hiding this comment

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

@dzucconi - you might be able to advise here

Copy link
Member

Choose a reason for hiding this comment

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

Wait what's this doing? We should just skip all the sticky stuff here; which should simplify things. I can take a closer look on Monday though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed up some updated code - from the designs, this rail should be stuck to the top on scroll

Copy link
Member

Choose a reason for hiding this comment

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

So yeah; I wouldn't use a Swiper here at all. Just use HorizontalOverflow

Copy link
Member

Choose a reason for hiding this comment

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

Ping on those docs @dzucconi https://palette.artsy.net/elements/horizontaloverflow/ 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, that definitely reduces code and complexity. Q for you @dzucconi - the scrollbar appears on HorizontalOverflow, and it seems like it shouldn't based on this line. When I inspect the bar, I actually don't see any scrollbar-width property set in the CSS (I would expect to see it on Viewport):

Screen Shot 2021-09-23 at 2 23 44 PM

Any idea what might be up here? IMO this shouldn't block this PR, but would be nice to sort out.

@mdole mdole force-pushed the mdole/FX-3173/categories-palette-v3 branch 4 times, most recently from 96253b6 to ed8a2f9 Compare September 21, 2021 17:21
@mdole mdole requested a review from dzucconi September 21, 2021 17:30
@mdole mdole marked this pull request as ready for review September 21, 2021 17:30
@mdole mdole mentioned this pull request Sep 21, 2021
Closed
damassi
damassi reviewed Sep 21, 2021
View reviewed changes
src/v2/Apps/Categories/Components/StickyNav.tsx Outdated
const { geneFamiliesConnection } = props
const geneFamilies = extractNodes(geneFamiliesConnection)
const { mobile, desktop } = useNavBarHeight()
const isMobile = useMatchMedia(themeProps.mediaQueries.xs)
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to avoid this as it's not SSR friendly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, thanks for calling out! I replaced it with this and this on the parent - so the parent is now using <Media at...> to determine whether it should pass down the mobile or desktop nav bar height. It's a smidge messy, but seems OK 🤷

@artsy artsy deleted a comment from ArtsyOpenSource Sep 21, 2021
anandaroop
anandaroop reviewed Sep 21, 2021
View reviewed changes
Copy link
Member

@anandaroop anandaroop left a comment •
edited

Choose a reason for hiding this comment

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

So nice to see this hacky hackathon hack grow up and become a real app ☺️. Wonderfully organized PR too.

I checked this out locally to take it for a spin and one thing confused me though — I couldn't seem to horizontally swipe the pill navigation. Just me?

src/v2/Apps/Categories/Components/CategoriesIntro.tsx Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@mdole mdole force-pushed the mdole/FX-3173/categories-palette-v3 branch from ed8a2f9 to a4db972 Compare September 22, 2021 16:12
@ArtsyOpenSource
Copy link

ArtsyOpenSource commented Sep 22, 2021 •
edited

Warnings
⚠️

Routes added to routes.tsx should have a corresponding cypress.js smoke test. See the cypress/integration folder for examples.

Generated by 🚫 dangerJS against 53096d7

@mdole mdole force-pushed the mdole/FX-3173/categories-palette-v3 branch from a4db972 to 3ba6beb Compare September 22, 2021 16:21
@mdole mdole force-pushed the mdole/FX-3173/categories-palette-v3 branch from 62327e1 to 3b41d13 Compare September 23, 2021 12:36
@mdole
Copy link
Contributor Author

mdole commented Sep 23, 2021

(totally non-blocking): Not sure if this was part of the comps, but this spacing feels a bit off to my eye, in that the margin isn't even between the top and the bottom, and so the category pills feel like they're adrift in space:

Evened out that spacing and updated the screenshots!

@dzucconi
Copy link
Member

Regarding this:

I think you'll have to give HorizontalOverflow a positive vertical padding and then an equal negative vertical margin

@dzucconi
Copy link
Member

Regarding the scrollbar:

I'm a little puzzled by this myself. I haven't looked into it much yet. Just ignore for now and I'll track down the root cause.

@damassi
Copy link
Member

damassi commented Sep 23, 2021

🚢

@mdole
Copy link
Contributor Author

mdole commented Sep 24, 2021

Regarding this:

I think you'll have to give HorizontalOverflow a positive vertical padding and then an equal negative vertical margin

🤯

Much better, thanks!

Screen Shot 2021-09-24 at 10 52 05 AM

@mdole mdole assigned anandaroop and unassigned mdole Sep 24, 2021
@anandaroop
Copy link
Member

Looking great, gonna merge this 🚢

@anandaroop anandaroop merged commit 99ea1dd into master Sep 24, 2021
@anandaroop anandaroop deleted the mdole/FX-3173/categories-palette-v3 branch September 24, 2021 21:02
@artsy-peril artsy-peril bot mentioned this pull request Sep 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzucconi dzucconi dzucconi left review comments

@anandaroop anandaroop anandaroop left review comments

@damassi damassi damassi left review comments

@araujobarret araujobarret Awaiting requested review from araujobarret

Assignees

@anandaroop anandaroop

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mdole @damassi @ArtsyOpenSource @dzucconi @anandaroop