Add Saved Segments UI (variant D) #4891
Conversation
|
apata
force-pushed
the
saved-segments/variant-d
branch
from
e6af6e9 to
c43f1c6
Compare
|
apata
force-pushed
the
saved-segments/variant-d
branch
3 times, most recently
from
fc60610 to
ef8670a
Compare
|
apata
force-pushed
the
saved-segments/variant-d
branch
2 times, most recently
from
567faec to
5710785
Compare
apata
force-pushed
the
saved-segments/variant-d
branch
from
f5bde8e to
2c3e8e6
Compare
apata
changed the title
| export function dateForSite(utcDateString, site) { | ||
| return dayjs.utc(utcDateString).utcOffset(site.offset / 60) | ||
| } |
There was a problem hiding this comment.
This is the new thing here, and it's a workaround. To follow the rest of the backend, I'd have to offset the dates there.
The other changes are related to formatting.
| @@ -397,6 +411,30 @@ defmodule PlausibleWeb.StatsController do | |||
| end) | |||
| |> Map.new() | |||
|
|
|||
| defp get_members(site_role, %Plausible.Site{} = site) | |||
There was a problem hiding this comment.
This is a workaround to avoid resolving segment.owner_id to segment.owner_name in the backend. Instead, we forward users <id>:<name> map to the front-end.
There was a problem hiding this comment.
Why not resolve it on the backend?
There was a problem hiding this comment.
Mostly because this worked ok and the PR was getting too bloated already.
There was a problem hiding this comment.
Got it. I think the logic would be more cohesive if the owners were resolved in the same query as segments themselves. But I agree with not making the PR any bigger than it is.
| className={classNames( | ||
| 'self-start button h-9 !px-3 !py-2 flex !bg-red-500 dark:!bg-red-500 hover:!bg-red-600 dark:hover:!bg-red-700 whitespace-nowrap' | ||
| 'button flex self-start h-9 !px-3 !bg-red-500 dark:!bg-red-500 hover:!bg-red-600 dark:hover:!bg-red-700 whitespace-nowrap' |
There was a problem hiding this comment.
I think the .button class should be considered legacy and not used for any new stuff. This applies to pretty much everything in app.css. These were added due to missing features in tailwind that would not allow us to use tailwind directly. But tailwind has been developed a lot and pretty much everything is possible now by just using tailwind classes.
There was a problem hiding this comment.
True, I saw that it was sort of wonky, and I'd be ok with getting rid of it. At the moment, I think let's leave it to mark which components should ideally share style.
Separate topic, but I had a notion that these custom classes made using TW @apply in app.css could be used to share class combinations between server-rendered pages and React dashboard.
Now that we have the storybook as well, it's especially tempting to try to codify and share these basic styles, like texts, notices, button types, dropdown backgrounds.
There was a problem hiding this comment.
Hmm yes they could be used for sharing...
For me though, one of the biggest things I like about tailwind is not having to come up with class names, and especially names for modifier classes :) I prefer the style where template and style code are combined and named together as a component (like <.button theme={"plain"} disabled="true"> over .button .button--plain .button--disabled).
| return '(Removed User)' | ||
| } | ||
|
|
||
| // if (owner_id === user.id) { |
There was a problem hiding this comment.
Should this be uncommented? Deleted?
| @@ -397,6 +411,30 @@ defmodule PlausibleWeb.StatsController do | |||
| end) | |||
| |> Map.new() | |||
|
|
|||
| defp get_members(site_role, %Plausible.Site{} = site) | |||
There was a problem hiding this comment.
Why not resolve it on the backend?
Changes
Adds saved segments related menus and modals. Almost all of it is conditional on feature flag saved_segments: true.
Tests
Changelog
Documentation
Dark mode