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

Adds sticky header row (site selection, date, filters) #472

Merged
merged 39 commits into from
Dec 22, 2020

Conversation

Vigasaurus
Copy link
Contributor

@Vigasaurus Vigasaurus commented Dec 15, 2020

Changes

Added functionality for the site selector, date, and filter section to be pinned (made sticky), with some user choice for pinning behavior in the event that the screen is too small for pinning.

This PR is built on top of #467 - i.e. it supports dark mode and should only be merged after #467.

Fixes #409

I've only been able to test this thoroughly on Chrome, FF, and their Android counterparts - please do be sure to test this on Safari as well, as Safari seems to be tempramental with position: sticky at times.

Tests

  • This PR does not require tests
    Front-end only

Changelog

  • Entry has been added to changelog

Documentation

  • Docs have been updated
  • This change does not need a documentation update
    Unsure if this needs a doc update - I'd lean to no since it's somewhat intuitive, but could do.

Demo

Demo Video

This was a pain to test D:
(they looked bad, on second thought)
The PR number could be wrong, will double check
As I said, my Elixir is pretty weak heh
@ukutaht
Copy link
Contributor

ukutaht commented Dec 15, 2020

(of course the replica would only contain live demo stats. Info that is public by default)

@Vigasaurus
Copy link
Contributor Author

Yeah I figured it wouldn't be super useful for most - I do most of my dev environments on a dedicated VPS so I end up throwing nginx in front of it, but that's definitely not the norm heh.
I'd definitely be interested in anything more we could get to mock dev data - even if that was just some script to populate completely random data more eloquent than make dummy_event

@Vigasaurus
Copy link
Contributor Author

Once we get #467 merged I'll go back through for the conflicts here (I have a hunch #467 will fix most of them anyways)

@ukutaht
Copy link
Contributor

ukutaht commented Dec 16, 2020

I think ideally I would set up a read-only replica Clickhouse instance. So when working locally you could connect to a remote Clickhouse database that was keeping up to date with the live demo in realtime. That way you could also make changes to the server code and preview them locally.

Unfortunately setting up Clickhouse replication requires running Zookeeper: https://clickhouse.tech/docs/en/engines/table-engines/mergetree-family/replication/

Adding zookeeper would make our infra more complicated than it needs to be so this will have to wait

@Vigasaurus
Copy link
Contributor Author

Shame. I think the next best thing as an intermediate solution would be to have a way to spoof the date/time on an event (maybe only in dev mode), and then it'd be easy to write a script that just sends a series of spoofed events over the past x days. That could at least make it easy to put together any set of test data as needed.

@ukutaht
Copy link
Contributor

ukutaht commented Dec 16, 2020

Awesome job on the dark theme @Vigasaurus! Once conflicts are merged I will test this one out locally and leave feedback.

@Vigasaurus
Copy link
Contributor Author

Vigasaurus commented Dec 16, 2020

I think this is ready to go too @ukutaht - I do definitely think that realtime.js:31-55 and historical.js:31-56 could be split off a bit to prevent the repetition between them, but at the same time passing props down that deep is likely worse than a bit of repetition.

@ukutaht
Copy link
Contributor

ukutaht commented Dec 18, 2020

OK I pulled and tested it locally, here's my feedback:

  • The header does server its purpose. I feel more oriented in the dashboard and I have more navigation options available when scrolled down 👍
  • I think we should remove the concept of pinning the header. One of the main aspects that people like about Plausible is the uncluttered UI and we want to keep it as clean as possible. If the header makes the dashboard unusable to the point that the user needs to unpin it, we should find a way to make it less offensive or not implement one at all.
  • The text within the sticky header is not currently vertically centered. The sticky element currently has pt-4 pb-2. It can be centered by usign py-3 instead.
  • We can use tailwind utilities in the custom css file. This worked great:
.fullwidth-shadow::before {
  @apply absolute w-screen top-0 h-full bg-gray-50 border-b border-gray-300 shadow;
  content: " ";
  z-index: -1;
  left: calc(-1 * calc(50vw - 50%));
}
  • As a future improvement, adding a filter should not make the header grow. I think we can shuffle things around in the header a bit to accomplish this.

@Vigasaurus
Copy link
Contributor Author

Vigasaurus commented Dec 18, 2020

  • The header does server its purpose. I feel more oriented in the dashboard and I have more navigation options available when scrolled down 👍

That was the goal so yay :D

  • I think we should remove the concept of pinning the header. One of the main aspects that people like about Plausible is the uncluttered UI and we want to keep it as clean as possible. If the header makes the dashboard unusable to the point that the user needs to unpin it, we should find a way to make it less offensive or not implement one at all.

I feel that, makes sense definitely

  • The text within the sticky header is not currently vertically centered. The sticky element currently has pt-4 pb-2. It can be centered by usign py-3 instead.

👍

  • We can use tailwind utilities in the custom css file. This worked great:
.fullwidth-shadow::before {
  @apply absolute w-screen top-0 h-full bg-gray-50 border-b border-gray-300 shadow;
  content: " ";
  z-index: -1;
  left: calc(-1 * calc(50vw - 50%));
}

Neat! I didn't actually know that was a thing at all

  • As a future improvement, adding a filter should not make the header grow. I think we can shuffle things around in the header a bit to accomplish this.

Yeah that'd be cool - I'll look into options in a later PR. Maybe something like:

| Site Name | Current Viewers | Something else? |
| --------------------Filters----------- | ---- Blank --------- |
| --------------------Filters----------- | Date/Time Nav |

Will make the changes here 👍

* Removes option to unsticky
* py-3
* Tailwind in CSS
@ukutaht
Copy link
Contributor

ukutaht commented Dec 18, 2020

Hey, I took the liberty to make the header fit on a single line when using filters. I will merge this first and then merge my own work on top of that. Thank you so much for the help!!

Commit: 1fe7c6e
Preview: https://staging.plausible.io/plausible.io/

@Vigasaurus
Copy link
Contributor Author

Neat! I notice a potential spot for awkwardness in the mobile version of what you've got - but once they're both merged I'll double check if it persists :D

@ukutaht
Copy link
Contributor

ukutaht commented Dec 18, 2020

Yeah, there are a lot of opportunities to improve mobile UX. The nav is probably the worst offender but there are other issues as well. I'll take a look.

@Vigasaurus
Copy link
Contributor Author

Vigasaurus commented Dec 18, 2020

Yeah - I'm almost done with the first pass of the client-side exclusions, once I finish that I plan to take a big whack at the UI and accessibility in general. Most of it will likely be mobile, but will see.

On this note of filters btw, I've always felt like there would be benefit to a "clear-all" for filters - but I may be in the minority for how many filters I often end up with applied (I have probably too many goals) - I'd guess most people end up with no more than 3-4 without losing meaningful data so it's probably no big deal. Thoughts?

@ukutaht
Copy link
Contributor

ukutaht commented Dec 18, 2020

Yeah - I'm almost done with the first pass of the client-side exclusions, once I finish that I plan to take a big whack at the UI and accessibility in general. Most of it will likely be mobile, but will see.

Ah OK, I won't touch the UI for now then.

On this note of filters btw, I've always felt like there would be benefit to a "clear-all" for filters - but I may be in the minority for how many filters I often end up with applied (I have probably too many goals) - I'd guess most people end up with no more than 3-4 without losing meaningful data so it's probably no big deal. Thoughts?

Sounds useful to me, but as always I'm worried about adding UI elements. If it can fit and feel natural then I'm all for it, really depends how it looks and feels. Some ideas:

  • Could try to fit it into the sticky nav somewhere
  • Could be some sort of floating action button
  • Could just be mapped to a key like ESC for power users.

@Vigasaurus Vigasaurus closed this Dec 18, 2020
@Vigasaurus Vigasaurus reopened this Dec 18, 2020
@Vigasaurus
Copy link
Contributor Author

If it can fit and feel natural then I'm all for it, really depends how it looks and feels.

Yeah that makes sense - I think best bet would be just a single X or super minimal other button maybe that we only show after x number of filters are applied - the hard part with a lot like this though is that the mobile UI is already pretty full and it's hard to not start overflowing.

I will say though, I'd imagine most users use the dashboard most often on desktop viewports (if only we had a way to track that 😏), thus a more extensive keyboard navigation setup could be quite nice - the biggest opportunities I see are for time period swapping (d,w,m,r or similar) and maybe site switching (maybe something like ctrl+right/left) - but that may be too "power-user-y" for the normal user

@Vigasaurus
Copy link
Contributor Author

Also, I mocked up a potential filter UI/UX re-do, to try to cut down on the UI variability with the number of filters, and make it easily mobile-friendly. Would love your thoughts before I put it together in my UI pass 👍
image

@Vigasaurus
Copy link
Contributor Author

@ukutaht Whenever you get a chance, if you want, you could merge your changes to the filters into this branch and I can look into making the mobile changes as suggested in the Twitter poll responses here directly. (Potentially the filter drop-down in the above comment, as well as maybe dropping the padding a bit on mobile to reduce the height) - lmk thoughts.

@ukutaht
Copy link
Contributor

ukutaht commented Dec 21, 2020

@Vigasaurus yes I will merge this PR and put my own changes on top later today/early tomorrow.

From your previous comment I like the idea of combining filters into one dropdown but if and only if they don't fit on the screen normally. We could use this as the mobile UX because there's no room on mobile anyways. On desktop I personally prefer seeing the actual filters.

Would be great if you could improve the mobile UX for the sticky nav and filters. Playing with paddings is good idea, I haven't experimented with that on mobile. Could also try reducing font-size in some areas to gain more space. But give me a bit of time to merge all the changes before you get started with that, otherwise we'll have a merge conflict hell later on

@ukutaht ukutaht merged commit b64f9de into plausible:master Dec 22, 2020
@ukutaht
Copy link
Contributor

ukutaht commented Dec 22, 2020

@Vigasaurus Thanks!

I've merged this but I'd like to improve the mobile UX before deploying it live. If you'd like to pick it up let me know, otherwise I'm happy to do it as well :)

@Vigasaurus
Copy link
Contributor Author

Yeah sounds good. I'll take a whack at it later today/early tomorrow.

oliver-kriska pushed a commit to payout-one/analytics that referenced this pull request Dec 23, 2020
* Adds New Dark Mode Assets

* Moves triangle for dropdown to a reasonable position

* Majority .eex dark implementation

* Fixes Logo Positioning

* Adds theme flag to user schema, uses it

* Uses correct variables for theme applicator script

* Minor missed theme changes/fallbacks

* Individual Component Support + Theme Context

* Sources Tab Support

This was a pain to test D:

* Partial Stats Sections Support

* More of stats modules supported

* Modal +table support

* Improves some Flatpickr in light theme, supports dark theme

* Fixes missed settings tab colors

* Finishes Devices module support

* Fixes bar graph colors

* Better colorizes maps module

* Undoes colorized bars

(they looked bad, on second thought)

* Fixes loading indicator

* Finishes conversions module

* Adds changelog entry

The PR number could be wrong, will double check

* Fixes missed header color

* Fixes naming of migration and removes static alter

* Does migration correctly

As I said, my Elixir is pretty weak heh

* Adds support for spike notifications setting

* Improves contrast and visibility for email settings

* Add initial implementation for sticky header

* Minor mobile formatting improvements

* Finishes (probably) pinned header feature

* Adds changelog entry for pinned header

* Updates PR number in sticky header changelog entry

* Wraps up code cleanup + feature

* Merge conflicts are hard, apparently

* Removes extraneous tailwind variants

* Fixes mentioned issues

* Removes option to unsticky
* py-3
* Tailwind in CSS
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.

2 participants