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

Set width for calendar #17348

Merged
merged 6 commits into from Apr 7, 2021
Merged

Set width for calendar #17348

merged 6 commits into from Apr 7, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 16, 2021

Description:

fixes #17162

Tested in FF, Chrome and IE11.
Tested with responsive design mode as well.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz marked this pull request as draft March 16, 2021 04:23
@flamisz flamisz self-assigned this Mar 16, 2021
@flamisz flamisz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 16, 2021
@flamisz flamisz added this to the 4.3.0 milestone Mar 16, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 16, 2021

Couple of screenshots, to see how it looks like. The only issue is on mobile, when we have two calendars, you have to scroll horizontally. It was like this, I didn't touch that part, and probably that's all right. If it's all right, I can update the UI tests (probably will fail a couple).

Screen Shot 2021-03-16 at 5 26 39 PM

Screen Shot 2021-03-16 at 5 26 46 PM

Screen Shot 2021-03-16 at 5 26 56 PM

Screen Shot 2021-03-16 at 5 27 18 PM

Screen Shot 2021-03-16 at 5 27 23 PM

@flamisz flamisz marked this pull request as ready for review March 16, 2021 04:30
@flamisz flamisz added the Needs Review PRs that need a code review label Mar 16, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 16, 2021

Wondering if it might look a bit better if the period options would be placed with a fixed distance to the calendar.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 16, 2021

Wondering if it might look a bit better if the period options would be placed with a fixed distance to the calendar.

Yeah, probably. Currently, that's a table, which is not the best practise these days. I will look into it what's the quickest solution.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 17, 2021

I changed the table layout to flex. It looks like this now:

Screen Shot 2021-03-17 at 4 31 00 PM

This is a nicer solution.

Tested on IE11.

@sgiehl
Copy link
Member

sgiehl commented Mar 23, 2021

The code itself looks fine so far. The UI tests needs to be updated, but not sure regarding the UI overall. Maybe @tsteur or @mattab want to have a look

@tsteur
Copy link
Member

tsteur commented Mar 23, 2021

I'd actually prefer the original look and also how it was shown in Firefox be totally fine for me if the compare is over two lines (in some ways you could argue it's even better over two lines if there was a bit more gap on top of compare to)
image

Maybe the width of the calendar could be made only slightly wider and if it breaks in some languages we wouldn't mind because it would still kind of work.

Seems @mattab is not happy with the two lines though so be good to comment @mattab

@mattab
Copy link
Member

mattab commented Mar 25, 2021

Would it be possible to make it look like this?

ideal look - Screenshot from 2021-03-25 14-14-01

so same as #17348 (comment) but:

  • the calendar panel is less wide
  • the select list less wide
  • so everything fits neatly to the left

@tsteur
Copy link
Member

tsteur commented Mar 25, 2021

@mattab problem are also eg translations etc.

from a UX point of view the way you want it is btw some confusing because you look from top to bottom and you see various periods listed on the right and you look further down where it then suddenly says previous periods and it's not clear what/how that relates to. I would say to show the previous period select only when the checkbox is activated. It might cause some jumping but at least the majority of the use cases would be simplified and eg GA does the same etc (because this select is confusing where you want it placed).
image

@mattab
Copy link
Member

mattab commented Mar 25, 2021

Agreed, that would be a great improvement to show the ... to [ previous period ] after a click on [ ] Compare

@flamisz
Copy link
Contributor Author

flamisz commented Mar 26, 2021

@mattab and @tsteur to show only the [previous period] after ticking the compare sounds good to me.
My question is, do we want to change the layout in any way? Like make it always on the second line, or make it slightly wider or just keep it as it is, so someone got it in one line, someone 2 lines?

@mattab
Copy link
Member

mattab commented Mar 26, 2021

My question is, do we want to change the layout in any way

Ideally looks like this (imagine the checkbox should be clicked here, and the white space on the right should be the same width as on the left)

ideal look - Screenshot from 2021-03-25 14-14-01

@flamisz flamisz removed the Needs Review PRs that need a code review label Mar 26, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 26, 2021

Ideally looks like this (imagine the checkbox should be clicked here, and the white space on the right should be the same width as on the left)

Yeah, it will work, but only in English. So my next question is what about other languages? Like in German, the word is longer, so won't fit. I can make the fonts smaller so even longer words can fit, only the very long ones will break into 2 lines.
Or if the word is longer, we'd like to grow the box?
ping @mattab

@mattab
Copy link
Member

mattab commented Mar 29, 2021

Or if the word is longer, we'd like to grow the box?

for visual consistency it's better to preserve the box width, so instead you can do the other solution " long ones will break into 2 lines." 👍

@flamisz
Copy link
Contributor Author

flamisz commented Mar 29, 2021

I tweaked the css a little bit, works well with even German translation, tested it in IE11/Chrome/Firefox.

Screen.Recording.2021-03-29.at.6.54.19.PM.mov

@flamisz flamisz added the Needs Review PRs that need a code review label Mar 29, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Apr 6, 2021

👋 @mattab can you please check the latest version of this? There is a short video in my previous comment. Thanks

@mattab
Copy link
Member

mattab commented Apr 7, 2021

Looks good to me @flamisz 👍 Nice improvement

@flamisz flamisz merged commit bdae345 into 4.x-dev Apr 7, 2021
@flamisz flamisz deleted the 17162-compare-to-button branch April 7, 2021 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the "Compare to" button design has slightly regressed on firefox only
5 participants