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

feat(atomic): add functionality arrow button scroll #2130

Merged
merged 35 commits into from
Jun 28, 2022

Conversation

mbenkirane-coveo
Copy link
Contributor

@mbenkirane-coveo mbenkirane-coveo commented Jun 21, 2022

Demo:
https://user-images.githubusercontent.com/97299176/175075314-c8618633-4087-45cd-8536-3ec907dc0514.mov

  • Based off this PR
  • Still need to adjust pixelsScrolled according to window width (fixed)

@ThibodeauJF
Copy link
Contributor

I'll look this up, but before please make sure you have everything merged up and no conflicts with your previous changes 🙏

@mbenkirane-coveo
Copy link
Contributor Author

mbenkirane-coveo commented Jun 21, 2022

I'll look this up, but before please make sure you have everything merged up and no conflicts with your previous changes 🙏

I'll make sure to properly merge when KIT-1722-Segmented-Scrollable is merged to master

@ThibodeauJF
Copy link
Contributor

It should always be properly merged when asking for reviewers to look at it otherwise it's hard for us to keep track of what's happening

If you want:
master <- KIT-1722-Segmented-Scrollable <- KIT-1756-Add-functionality-arrow-scroll

You can just point this PR to the KIT-1722-Segmented-Scrollable branch
Screen Shot 2022-06-21 at 11 14 49 AM

@mbenkirane-coveo mbenkirane-coveo changed the base branch from master to KIT-1722-Segmented-Scrollable June 21, 2022 15:16
@mbenkirane-coveo
Copy link
Contributor Author

It should always be properly merged when asking for reviewers to look at it otherwise it's hard for us to keep track of what's happening

If you want: master <- KIT-1722-Segmented-Scrollable <- KIT-1756-Add-functionality-arrow-scroll

You can just point this PR to the KIT-1722-Segmented-Scrollable branch Screen Shot 2022-06-21 at 11 14 49 AM

sounds like a good solution to me

Base automatically changed from KIT-1722-Segmented-Scrollable to master June 21, 2022 16:22
@liviarett
Copy link
Contributor

liviarett commented Jun 22, 2022

I'd suggest that, instead of scrolling the full width, you snap to the closest 'full facet'. For example, look at group 15 here:
Screenshot 2022-06-22 at 12 47 28
Screenshot 2022-06-22 at 12 47 48
you never get to see its full value.

this is the behaviour I'd suggest:

when in the first page, show the full leftmost facet, like so
Screenshot 2022-06-22 at 12 50 19

when navigating with the arrows, show the full facet on the left
Screenshot 2022-06-22 at 12 52 51

not sure if I'd change the behaviour depending on the side (when navigating right, snap to the leftmost facet; when navigating left, snap to the rightmost facet); depends what it looks like, but essentially you want to make sure you can read the full thing at some point!

mbenkirane-coveo and others added 8 commits June 28, 2022 11:15
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…facet/atomic-segmented-facet.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
…crollable/atomic-segmented-facet-scrollable.tsx

Co-authored-by: jpmarceau <[email protected]>
@mbenkirane-coveo mbenkirane-coveo requested a review from a team as a code owner June 28, 2022 18:12
@mbenkirane-coveo mbenkirane-coveo enabled auto-merge (squash) June 28, 2022 18:16
Copy link
Contributor

@ThibodeauJF ThibodeauJF left a comment

Choose a reason for hiding this comment

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

Nice!

@ThibodeauJF ThibodeauJF merged commit 9b1b77b into master Jun 28, 2022
@ThibodeauJF ThibodeauJF deleted the KIT-1756-Add-functionality-arrow-scroll branch June 28, 2022 18:50
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.

5 participants