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

Rework settings UI #4626

Merged
merged 54 commits into from
Oct 2, 2024
Merged

Rework settings UI #4626

merged 54 commits into from
Oct 2, 2024

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Sep 26, 2024

This is a long-running work and a compromise reached when trying to make the settings UI a bit more consistent, extracting common components along the way. Contains some @ukutaht's work and addresses lots of review suggestions by @metmarkosaric - tons of context and evolutionary log can be found here

@aerosol aerosol changed the title UI stuff rebased Rework settings UI Sep 30, 2024
@aerosol aerosol mentioned this pull request Sep 30, 2024
8 tasks
@aerosol
Copy link
Member Author

aerosol commented Oct 1, 2024

@ukutaht don't want to keep this cooking for too long, kindly paging for approval - will address any issues potentially surfacing from it

Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Great work @aerosol

I agree that we should merge this quite quickly and not leave it in review for a long time.

I commented where I saw text-sm being removed with the assumption that it's probably a mistake from going to bigger fonts and back.

There is one thing about the components that I think should be changed. Namely the fact that some of these components have a built-in margin to them. I don't think reusable components should do that because positioning the component should be the concern of its parent. By default I think all components should be as neutral as possible about their positioning within the parent container or in relation to siblings.

Another question for me is whether to use these more specific attributes like width and mt?, or just have class like we do on some components. Difference being:

<.input width="w-1/2" mt?={true} />

vs just

<.input class="w-1/2 mt-2"/>

I think I prefer the second option of allowing the class attribute to be extended because it's the most flexible. What do you think?

I'm going to approve the PR because honestly none of this feedback is blocking and can be done in a follow-up.

lib/plausible_web/components/billing/billing.ex Outdated Show resolved Hide resolved
lib/plausible_web/components/billing/billing.ex Outdated Show resolved Hide resolved
lib/plausible_web/components/billing/billing.ex Outdated Show resolved Hide resolved
lib/plausible_web/components/billing/billing.ex Outdated Show resolved Hide resolved
lib/plausible_web/live/goal_settings/list.ex Outdated Show resolved Hide resolved
lib/plausible_web/live/props_settings/list.ex Outdated Show resolved Hide resolved
@aerosol
Copy link
Member Author

aerosol commented Oct 2, 2024

I commented where I saw text-sm being removed with the assumption that it's probably a mistake from going to bigger fonts and back.

Thanks, fixed via ab5e1fc

There is one thing about the components that I think should be changed. Namely the fact that some of these components have a built-in margin to them. I don't think reusable components should do that because positioning the component should be the concern of its parent. By default I think all components should be as neutral as possible about their positioning within the parent container or in relation to siblings.

Having spent some time wrestling with that dilemma, my reasoning was:

  1. Most extracted components live in a common context thus far, which is, at this
    point: site settings tile or a "focus box" (a modal, regardless of implementation)
  2. Before, the users were free to position them anywhere, and as a matter of
    fact, the components happened to be positioned randomly
  3. As a user (dev), I don't have any guidelines on how to position the
    component in relationship to its surroundings (such as
    headings, form elements etc.). I also don't believe devs know by heart
    when to use mt-1 or mt-4 or mt-8. The previous modus operandi was to
    copy relevant markup portions from elsewhere (where it could've been
    made inconsistent already).

Another question for me is whether to use these more specific attributes like width and mt?, or just have class like we do on some components. Difference being:

So, while I agree the whole execution of trying to abstract common
placements isn't amazing, I think at the "component" level, we could
benefit from a better interface than "classless classes" spaghetti.

I feel that maybe it's more natural to tell "this is too low, because it happens
to be the very first element, let's disable the top margin",
rather than try something arbitrary.

Many components have predefined "themes" or "defaults", and this is
great because the user isn't bothered with properties that shouldn't ever
change, e.g. input field border behaviour.

Power users like yourself will always prefer "class", because it's not
challenging for you to shuffle 50 mysterious strings in there, but the
problem with that lack of abstraction is, you can easily break the
resulting visual. What about components consisting of multiple
meaningful markup elements (simplest example: input with a label).
What will @class refer to, input or label or both? Does that mean,
we should make all the components accept slots?

<ComboBox/> has a @class attribute already; can you guess what can you customize with
it? For Team Accounts it looks like the dropdown elements will contain
images (user avatars). Is class "flexible" enough to let us handle that use case?

Last but not least, I can't stress this enough:
I don't think doing customizations via max_width="max-w-40"
is a pretty interface :D We're commited to TailWind, so this could be
max_width=40 resulting with class={[defaults, "max-w-40"]},
but I can't currently tell if that's any less controversial.

In the end, even if we ever get to a "storybook" level of standardisation,
this will be only applicable to a limited set of cases specific to
plausible.io, which is why I think flexibility is only nice to have, but
not mandatory.

And as for this PR, it does not exhaust all the work required
to get us there nor definitely fixes all the issues I mentioned.

image

WYT? @ukutaht

@aerosol aerosol added this pull request to the merge queue Oct 2, 2024
Merged via the queue into master with commit ec2a560 Oct 2, 2024
10 checks passed
@ukutaht ukutaht deleted the ui-stuff-rebased branch October 2, 2024 20:20
@ukutaht
Copy link
Contributor

ukutaht commented Oct 2, 2024

Default margins

Having spent some time wrestling with that dilemma, my reasoning was:

Yeah these are real problems. I agree with the need to standardize layouts and positioning.

But with the widespread browser support of flex and grid I think we should move towards invisible 'layout components' instead of baking layout into the visual components themselves.

visual components positioning themselves

<input class="mt-4" />
<input class="mt-4" />
<button class="mt-4" />

layout component, visual components agnostic of layout

<div class="flex flex-col gap-y-4">
  <input />
  <input />
  <button />
</div>

This keeps the visual components themselves more straightforward and composable because they can be used in any sort of layout.

I see the baked-in margins are used mostly if not exclusively in forms. I think adding a layout component that separates form fields with a gap-y-4 should give us the same standardization while keeping the input layout-agnostic which I think is a win.

Class attribute over width and mt?

It's getting too late, let's discuss another time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants