-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Updated empty states across settings #5874
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
base: master
Are you sure you want to change the base?
Conversation
|
- Add show_content? attribute to generic tile component - Ensure content is hidden when toggled off - Avoid rendering border and empty space when toggled off - Fix formatting
6ff4770 to
779dd92
Compare
| Add funnel | ||
| </.button> | ||
| </.filter_bar> | ||
| <%= if String.trim(@filter_text) != "" || Enum.count(@funnels) > 0 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.
or is perfectly valid here since both left and right hand side expressions return a boolean
- Update the settings live views to use the new tile component - Ensure tile component is updated when feature visibility is toggled - Extract `no_search_results` and `empty_state` components for better readability - Extract `highlighted` component - Update tests
73347e9 to
beefacd
Compare
aerosol
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.
Impressive work! Some comments inline.
| </.button> | ||
| </.filter_bar> | ||
| <% end %> | ||
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.
| <.filter_bar :if={@searching? or length(@props) > 0} filter_text={@filter_text} placeholder="Search Properties"> | |
| <.button phx-click="add-prop" mt?={false}> | |
| Add property | |
| </.button> | |
| </.filter_bar> |
when can props be anything else than a list here? 🤔
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.
The suggestion above displays in a weird way, apologies, probably not a great idea to commit it directly 😓
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.
Agreed with Adam, even though the condition existed already before this PR, it's safe to remove. I think this is some kind of leftover from when site.allowed_event_props (which can be nil, meaning that all props are allowed to display) was passed into this component directly. But I just checked that that's not the case anymore and @props gets the value of site.allowed_event_props || [].
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.
Ok cool, yeah I kept it as I wasn't sure if I was missing any context on that. Updated it to be the same as goals and funnels.
lib/plausible_web/live/sites.ex
Outdated
| <.search_form | ||
| :if={ | ||
| @has_sites? and | ||
| (Teams.setup?(@current_team) or @sites.entries != [] or @filter_text != "") |
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.
Why is Teams.setup?(@current_team) here?
If a team is setup and all its sites have been deleted, then perhaps it's pointless to show the search bar? The +Add button becomes broken then though.
With several new conditions here and there, we might want to test those cases to guard it from regression - removing that :if breaks zero tests.
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.
The reason is that I improved the personal sites empty state, so that it's hidden there when personal sites is empty and team sites isn't.

But I realise now that I should also fix up the empty state for team sites, and perhaps that can simplify the whole thing a bit (although we still want a different call to action on the personal sites empty state).
|
|
||
| test "search funnels input is rendered", %{conn: conn, site: site} do | ||
| setup_goals(site) | ||
| {:ok, _} = setup_funnels(site) |
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.
👌 ✨
- Hide top bar on `/sites` when empty state is shown - Extract empty state logic to a separate function - Show the same empty state for both personal and team sites, with different copy - extract search logic to a separate function - add tests for various empty states cases
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.
Brilliant work! 👏 Thanks for this @sanne-san @aerosol!
Loving the site_role/current_role -> current_user assigns being changed. Also played around on staging and everything was working super smooth.
Noticed this small UI thing btw (not related to this PR but should be a very easy fix):
| <div class={["border-b dark:border-gray-700 mx-6", if(not @show_content?, do: "hidden")]}></div> | ||
| <div class={["relative", if(not @show_content?, do: "hidden")]}> |
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.
Nice! 👌
| :if={@feature_toggle?} | ||
| feature_mod={@feature_mod} | ||
| module={PlausibleWeb.Components.Site.Feature.ToggleLive} | ||
| id={"feature-toggle-#{@site.id}-#{@feature_mod}"} |
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.
What's this id used for? I'm seeing another id attribute defined by the ToggleLive component itself (the wrapper div):
<div class="mt-4" id={"feature-#{@feature_mod.name()}-toggle"}>
Should we stick to a single one?
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.
Tests. I guess we prefer data-test-id?

Changes
Tests
Dark mode