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

fix class name typo and include it in safelist #2610

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

ruslandoga
Copy link
Contributor

Changes

Fixes #2514

This PR fixes the class name used for embedded dashboards and includes it in a safelist to avoid purging.

Tests

  • This PR does not require tests

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode

@ruslandoga ruslandoga requested a review from a team January 18, 2023 15:53
@@ -52,7 +52,7 @@ defmodule PlausibleWeb.StatsView do
def stats_container_class(conn) do
cond do
conn.assigns[:embedded] && conn.assigns[:width] == "manual" -> ""
conn.assigns[:embedded] -> "max-width-screen-lg mx-auto"
conn.assigns[:embedded] -> "max-w-screen-lg mx-auto"
Copy link
Contributor Author

@ruslandoga ruslandoga Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it was supported in tailwind v1 or v2 or if it's just a typo, but there doesn't seem to be a max-width-screen-lg class anymore: https://tailwindcss.com/docs/max-width


seems like it was a typo introduced in #2148

@bundlemon
Copy link

bundlemon bot commented Jan 18, 2023

BundleMon

Unchanged files (8)
Status Path Size Limits
static/css/app.css
515.18KB -
static/js/dashboard.js
297.78KB -
static/js/sentry-bundle-5.9.1-min.js
15.7KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link
Contributor

@vinibrsl vinibrsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks for adding to the safelist :)

@vinibrsl vinibrsl self-requested a review January 19, 2023 13:41
Copy link
Contributor

@vinibrsl vinibrsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern: the embed dashboard is quite difficult to test because people use it in very different setups. If this class wasn't being applied, what do you think of actually removing it to keep to same behavior?

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Jan 19, 2023

I know @metmarkosaric uses the embedded dashboard: https://markosaric.com/stats/, so we can ask him what to do :)

With the fix it looks like this:

Screenshot 2023-01-19 at 21 59 04

@metmarkosaric
Copy link
Contributor

looks the same to me? any difference that i don't see?

@ruslandoga
Copy link
Contributor Author

Right now it looks like this (full width):

Screenshot 2023-01-19 at 22 07 29

@metmarkosaric
Copy link
Contributor

ah ok so with this change we don't allow people to choose the width or if it's not specified we show narrow width rather than full width by default?

@ruslandoga
Copy link
Contributor Author

if it's not specified we show narrow width rather than full width by default

This.

@metmarkosaric
Copy link
Contributor

hmm i'm unsure if this breaks the UI and design of some people that are happy with the full width. is there a reason to change this at all? seems fine to me the way it is. full width by default but you get to pick a manual width if you prefer that. it's already documented too

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Jan 19, 2023

is there a reason to change this at all?

Full width by default is a bug: #2514.
This PR actually reverts a change in the defaults to the way it was before #2148.

it's already documented too

From https://plausible.io/docs/embed-dashboard#manual-width-mode:

Normally, the embedded dashboard will render with a couple internal CSS rules that contain the dashboard:

@metmarkosaric
Copy link
Contributor

ok cool. i guess it's fine if this change wasn't intentional but a mistake. it's not something we discussed either. i'll let Vini decide

@metmarkosaric
Copy link
Contributor

narrow by default does look nicer!

@vinibrsl vinibrsl merged commit e3f095a into plausible:master Jan 20, 2023
@vinibrsl
Copy link
Contributor

Thanks, @ruslandoga!

@ruslandoga ruslandoga deleted the iframe-embed-missing-class branch January 20, 2023 13:42
This pull request was closed.
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.

Manual width mode is always enabled
3 participants