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: Collapsable historical picker #6911

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 28, 2024

Description

Adds a collapsible time controller/picker using an accordion instead of the bottom sheet (which is woefully unmaintained and outdated).

It also changes all necessary core components such the Badge, TimeAverageToggle, TimeAxis and TimeSlider to match with the new visual design.

Closes: AVO-357

Preview

image image

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@github-actions github-actions bot added frontend 🎨 dependencies Pull requests that update a dependency file labels Jun 28, 2024
@VIKTORVAV99 VIKTORVAV99 requested a review from tonypls July 9, 2024 07:40
@VIKTORVAV99
Copy link
Member Author

This PR has already gone through multiple "design reviews" with @Alportan so design wise it should be good to go, now it's just the code that needs some reviewing!

@tonypls
Copy link
Collaborator

tonypls commented Jul 9, 2024

No more sun and moon and night indicator on the slider?

@tonypls
Copy link
Collaborator

tonypls commented Jul 9, 2024

On iPhone SE for example, is this enough room now that the slider takes up more?
image

@VIKTORVAV99
Copy link
Member Author

No more sun and moon and night indicator on the slider?

There are, just not custom svgs as backgrounds, they are imported icons now to reduce the amount of http requests we make as we will always need them.

@VIKTORVAV99
Copy link
Member Author

On iPhone SE for example, is this enough room now that the slider takes up more? image

@Alportan can you give your thoughts here?

@Alportan
Copy link
Contributor

Alportan commented Jul 9, 2024

On iPhone SE for example, is this enough room now that the slider takes up more? image

The new designs brings more value for most users by allowing them to have instant access to the slider. Looking at our analytics, the oldest iOS version is 16.1 and the 1st gen SE runs up to iOS 15, which means we have 0 to little iPhone SE (1st gen) using our app.

As an alternative, if we want to optimise for this use-case, I would keep the design as is for all the other device sizes, and add a new breakpoint that removes the slider as well when collapsed on resolutions under 380 px width.

Hope that makes sense! 🙌

@tonypls
Copy link
Collaborator

tonypls commented Jul 9, 2024

On iPhone SE for example, is this enough room now that the slider takes up more? image

The new designs brings more value for most users by allowing them to have instant access to the slider. Looking at our analytics, the oldest iOS version is 16.1 and the 1st gen SE runs up to iOS 15, which means we have 0 to little iPhone SE (1st gen) using our app.

As an alternative, if we want to optimise for this use-case, I would keep the design as is for all the other device sizes, and add a new breakpoint that removes the slider as well when collapsed on resolutions under 380 px width.

Hope that makes sense! 🙌

You can still buy an iPhone SE from apple today and I believe Pierre has one as his primary device.

It's a nice improvement for desktop users but I feel it's a downgrade in viewing space and interactivity / animation on mobile.

Would it make sense to improve these on mobile:

  • Remove the "at" and make sure it the date doesn't become large than our icon on smaller devices. I would suggest using month trimming to 3 characters because the problem will get much worse in September. Perhaps we could use the previous data formatting and maybe drop off a comma and append the timezone similar to this:
image

Sizing issue on smaller devices:

image
  • I think on all mobile devices it would be nice to be able to collapse the slider and maximize map / chart viewing area, it feels bad to take this away when we have that functionality now. It would be especially nice if this could be with a touch gesture.

There's also a date overflow bug here:
image

P.S. Sometime since the last mobile release there is a bug where the slider dissapears completely after navigating to a zone page.

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 9, 2024

On iPhone SE for example, is this enough room now that the slider takes up more? image

The new designs brings more value for most users by allowing them to have instant access to the slider. Looking at our analytics, the oldest iOS version is 16.1 and the 1st gen SE runs up to iOS 15, which means we have 0 to little iPhone SE (1st gen) using our app.
As an alternative, if we want to optimise for this use-case, I would keep the design as is for all the other device sizes, and add a new breakpoint that removes the slider as well when collapsed on resolutions under 380 px width.
Hope that makes sense! 🙌

You can still buy an iPhone SE from apple today and I believe Pierre has one as his primary device.

It's a nice improvement for desktop users but I feel it's a downgrade in viewing space and interactivity / animation on mobile.

Would it make sense to improve these on mobile:

  • Remove the "at" and make sure it the date doesn't become large than our icon on smaller devices. I would suggest using month trimming to 3 characters because the problem will get much worse in September. Perhaps we could use the previous data formatting and maybe drop off a comma and append the timezone similar to this:
image

This is not a date that is formatted by us but rather the Intl builtin in JavaScript so it adapts to all regions and language variations included in this. We can't remove the at here without dumping that whole system.

The top datetime will be removed and replaced by an info and settings buttons in a upcoming pr, I think this is fine for now.

Sizing issue on smaller devices:

image
  • I think on all mobile devices it would be nice to be able to collapse the slider and maximize map / chart viewing area, it feels bad to take this away when we have that functionality now. It would be especially nice if this could be with a touch gesture.

As for the collapsible thing I see your point and if @Alportan agrees I'll change it.

There's also a date overflow bug here: image

Was introduced in a separate PR but this is addressed in #6969

P.S. Sometime since the last mobile release there is a bug where the slider dissapears completely after navigating to a zone page.

I noticed that as well, hopefully this will take care of that.

@Alportan
Copy link
Contributor

Alportan commented Jul 9, 2024

You can still buy an iPhone SE from apple today and I believe Pierre has one as his primary device.

I see now that the default SE in the inspector is the 2nd and 3rd gen. I see that they have a different iPhone 5/SE which is probably the old 1st gen. Didn't know that!

Would it make sense to improve these on mobile:

  • I think on all mobile devices it would be nice to be able to collapse the slider and maximize map / chart viewing area, it feels bad to take this away when we have that functionality now. It would be especially nice if this could be with a touch gesture.

We could start with the slider fully collapsed on all mobile devices and do some more investigation on this in the future. That's fine by me! 👌

Maybe one clarifying question @tonypls , what do you mean by "It would be especially nice if this could be with a touch gesture."? Do you refer to the old way users could swipe the card down to collapse/expand?

@tonypls
Copy link
Collaborator

tonypls commented Jul 9, 2024

Had a quick chat with Alex to understand the solution a bit more and we agreed on a few tweaks!

The main motivation is that users don't feel like the experience is diminished in anyway, mostly screen space on mobile.

  • Allow the picker to collapse on mobile to just the date view
  • Add a touch listener so if a user attempts to open the picker in the same manner as the current bottom sheet it will still open
  • Add some animation or transition on opening / collapsing of the picker 😎

Let me know if I can clarify further and feel free to suggest a better approach 🙏

@VIKTORVAV99
Copy link
Member Author

Had a quick chat with Alex to understand the solution a bit more and we agreed on a few tweaks!

The main motivation is that users don't feel like the experience is diminished in anyway, mostly screen space on mobile.

  • Allow the picker to collapse on mobile to just the date view
  • Add a touch listener so if a user attempts to open the picker in the same manner as the current bottom sheet it will still open
  • Add some animation or transition on opening / collapsing of the picker 😎

Let me know if I can clarify further and feel free to suggest a better approach 🙏

Are there not already an animation for the Accordion?

Otherwise I'll can add som react spring animations to match the old one more or less.
And the touch listener should not be a problem but I assume you want to add some "drag indicator" as well?

@tonypls
Copy link
Collaborator

tonypls commented Jul 9, 2024

Had a quick chat with Alex to understand the solution a bit more and we agreed on a few tweaks!
The main motivation is that users don't feel like the experience is diminished in anyway, mostly screen space on mobile.

  • Allow the picker to collapse on mobile to just the date view
  • Add a touch listener so if a user attempts to open the picker in the same manner as the current bottom sheet it will still open
  • Add some animation or transition on opening / collapsing of the picker 😎

Let me know if I can clarify further and feel free to suggest a better approach 🙏

Are there not already an animation for the Accordion?

Otherwise I'll can add som react spring animations to match the old one more or less. And the touch listener should not be a problem but I assume you want to add some "drag indicator" as well?

I'll let you find an animation if there isn't one!

It doesn't have to have the drag indicator, I think we should avoid pre-emptively building our own bottom sheet but if it's easy to add a listener that will work then I think we should include that!

@Alportan
Copy link
Contributor

Alportan commented Jul 9, 2024

Are there not already an animation for the Accordion?

After Tony mentioned that something feels different from the old implementation and I paid more attention and an animation when expanding/collapsing is missing 🙈 I double checked the Accordion and it doesn't have one either.

Otherwise I'll can add som react spring animations to match the old one more or less. And the touch listener should not be a problem but I assume you want to add some "drag indicator" as well?

I think it's fine not to add the indicator, as long as people try to drag the section and it works, I think it'll be fine! 👌 We want to keep the old mental models and not disrupt the way people interact with the app.

@VIKTORVAV99
Copy link
Member Author

I'll put this back in a draft and see how easy it is to implement these changes with the existing accordion components, but if we want touch gestures it might be simpler to just do our own bottom sheet for mobile to not get a weird flow when the accordion expands. Shouldn't be too much of an issue though.

And I might split up these into separate PR, like adding the animation to the base accordion in one standalone PR.

@VIKTORVAV99 VIKTORVAV99 marked this pull request as draft July 9, 2024 14:36
@madsnedergaard
Copy link
Member

madsnedergaard commented Jul 10, 2024

For the animation part, see the accordion in FAQContent.tsx! :) I honestly think it might be easiest/best to just migrate our own "Accordion" to Radix like we did there.

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 10, 2024

For the animation part, see the accordion in FAQContent.tsx! :) I honestly think it might be easiest/best to just migrate our own "Accordion" to Radix like we did there.

I think we are already using radix as the base for our own accordion as well. But I'll take a look.

EDIT:
We are not using Radix Ui for this accordion and tbh that looks like the right choice as radix seems more designed to use multiple accordions in a group than how we are using this.

@madsnedergaard
Copy link
Member

For the animation part, see the accordion in FAQContent.tsx! :) I honestly think it might be easiest/best to just migrate our own "Accordion" to Radix like we did there.

I think we are already using radix as the base for our own accordion as well. But I'll take a look.

EDIT: We are not using Radix Ui for this accordion and tbh that looks like the right choice as radix seems more designed to use multiple accordions in a group than how we are using this.

While it also handles multiple accordings greatly, I think it can work just fine for one - and give us better accessibility, transitions/animations and reduce the amount of code and edge cases we have to manually maintain ;)

@VIKTORVAV99
Copy link
Member Author

How would you guys feel about me splitting up this PR and bringing over some of the visual changes that don't need to change the bottom sheet or accordion itself into a new PR?

Should make this one easier to review when ready and give us most visual design changes sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collapsible time controller
4 participants