-
Notifications
You must be signed in to change notification settings - Fork 8
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
As editors we would like to use a horizontal scroll for our promotion components on desktop and mobile to promote more content and take up less vertical real estate #1373
Comments
As discussed with @SvSven. FYI @meols
How does EVENTS and PEOPLE factor in? |
For future post-Christmas self:
Left to do:
For testing: https://web-global-development-equinor-web-sites-dev.c2.radix.equinor.com/test/carousel |
@SvSven Firstly, great job, this is looking very good on the components it has been created for.. Small comment from me, Can the Left arrow button display as soon as the Right button been clicked. Currently it only appears when the user reaches the end of the scroll (you can see this on your TOPIC promotion test page) |
Hi @SvSven, I haven't looked at the code yet. Just tested the UX. I think it is looking very nice! One thing that I personally dislike tho is the arrow buttons displayed on mobile view, but that might be just me 😅 Other than that I found a bug where some of the cards are shrinking inside the carousel (I believe to respect a possible max-width limit?). When this gets fixed I'll dive deeper in the code :d |
Yeah I think those arrows are going to be the death of me. @NickHaggerty1 I'll have a look to see if I can tweak the showing/hiding of the navigation arrows a bit, currently it's doing some automagic things for it. Also the difference in card image; that's weird, they should all be the same size 🤔 |
@fernandolucchesi @NickHaggerty1 navigation arrows should now properly show/hide and the cards should all be the same width as well |
@SvSven You have shown them buttons who's boss thats for sure! Nice work |
I think the width should vary accordingly to the carousel type: |
@fernandolucchesi the issue with the people promotion card width should be fixed now. We might want to refactor the implementation slightly when we add support for iframes in a horizontal scroll, but for now it should be fine. @NickHaggerty1 To try and move things along a bit better, I made a separate issue for adding the horizontal scroll the to promo tile component, see: #1485 |
looks good! approved! ✅ |
Approved. @fernandolucchesi @meols But there is a small bug that maybe is only on Stage? The EVENTS promotion comes with a warning to select a tag. Even through I have selected to manually choose my events. |
@NickHaggerty1 currently looking into this |
@NickHaggerty1 this happens when you toggle manually choose events, when select by tag toggle is still toggled - even though it is hidden. I will try to make a quick fix for this |
Nice thanks @millianapia |
@NickHaggerty1 should work as expected, could you verify before we move this issue to the deploy column |
@millianapia Good stuff. Approved, works as expected. Thanks |
@NickHaggerty1 i also made it so the navigation buttons now are hidden when used on a phone size, hope it looks ok |
Yeah thats great. Lets go for that @millianapia until someone complains about the lack of arrows! Thanks for fixing. Approved |
Acceptance criteria
Inspiration:
https://www.ikea.com/no/no/ (third module down)
https://www.apple.com/environment/ (bottom of page)
The text was updated successfully, but these errors were encountered: