-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Consolidated View life cycle + billing integration #5866
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
Conversation
ea6644d to
b1e1b17
Compare
2fa36cd to
1b9decd
Compare
- add functions to manipulate user/team options (for CTA) - require at least two sites in order to create a consolidated view - require billing/plan compliance when computing eligibility
Co-authored-by: Sanne de Vries <[email protected]> Co-authored-by: Uku Taht <[email protected]>
- require team-wise feature flag instead of super admin role - redirect to /sites if the team isn't eligible any more - enforce regular site in shared links controller
1b9decd to
87e593f
Compare
64760be to
2ae4797
Compare
RobertJoonas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, looks great overall! A few comments inline.
extra/lib/plausible_web/live/customer_support/team/components/consolidated_views.ex
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| def upgrade_card(assigns) do | ||
| def consolidated_view_card_cta(assigns) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a slight UI annoyance I'm seeing locally. Try to hide the CTA card, then navigate to another page, e.g. click on one of the site cards to see a dashboard. Now, hit back in the browser history. The card flickers for a second and then disappears. Same happens when you've hidden it before and then restore it.
We should make sure that hiding/restoring the card also updates the entry in browser history stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phoenixframework/phoenix_live_view#3508 looks like a similar issue and from what I see it was tricky to address and I'm not event sure it got fully resolved?
| <div :if={@has_sites?}> | ||
| <ul class="my-6 grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3"> | ||
| <!-- Insert upgrade_card here --> | ||
| <.consolidated_view_card_cta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can extract this to a separate component and maybe even extract the state management out of here. It's getting really noisy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving conditional expressions to the start of render and assigning them to something like show_consolidated_view_cta? and show_consolidated_view? might help clean it up as well.
557d2e8 to
f2b5a2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God's work 🙏 Left some feedback, most of it just nice-to-haves. Keeping preferences on membership instead of team/user combo makes more sense to me.
EDIT: Oh, and one more thing I've noted and then forgot about. Perhaps ConslidatedView.reset_if_enabled could be turned into an upsert? That would pair well with disable which is resilient to the view struct getting stale already.
| <Heroicons.cog_6_tooth class="hidden group-hover:inline size-5 dark:text-gray-100 text-gray-900" /> | ||
| </.unstyled_link> | ||
| </h2> | ||
| <.unstyled_link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a good opportunity to use icon_button component, which was recently introduced? 😄 Though it might require introducing one more theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrm @sanne-san do you think it's something we should do in this context?
| <div :if={@has_sites?}> | ||
| <ul class="my-6 grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3"> | ||
| <!-- Insert upgrade_card here --> | ||
| <.consolidated_view_card_cta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving conditional expressions to the start of render and assigning them to something like show_consolidated_view_cta? and show_consolidated_view? might help clean it up as well.
| end | ||
|
|
||
| def upgrade_card(assigns) do | ||
| def consolidated_view_card_cta(assigns) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phoenixframework/phoenix_live_view#3508 looks like a similar issue and from what I see it was tricky to address and I'm not event sure it got fully resolved?
Will be included via main branch sync This reverts commit 05bec55.
|
Changes
This PR integrates consolidated view life cycle with billing properties.
It relies on two migrations (
included here still), extracted to separate PRs:Main changes:
:consolidated_view- we no longer display anything based onsuper_adminrole; flag is not setup on production yet, meaning existing consolidated views will disappear (and no CTAs will be shown).ConsolidatedView/sitesis visited an attempt is made to create consolidated view. If the team is eligible, and the feature flag is raised, the consolidated view card is shown. Otherwise a CTA card is displayed (courtesy of @sanne-san)/siteswill include the consolidated view card."+ New consolidated view"dropdown item)/sitesConsolidatedView.enabled?/1has been removed, since enabling doesn't mean availabilityTODO:
there are visual issues with card plots and overlapping dropdownsrecord-2025-11-10-10-29-34-year.node.norm.mp4
the dropdown doesn't respect dark modeTests
Changelog
Documentation
Dark mode