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

luci-base: refactoring overlay for connectivity change #7152

Closed
wants to merge 1 commit into from

Conversation

feckert
Copy link
Member

@feckert feckert commented Jun 6, 2024

Before with LuCI-Theme bootstrap:
before

After with LuCI-Theme bootstrap:
after

  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • ( Preferred ) Mention: @ the original code author for feedback
  • ( Preferred ) Screenshot or mp4 of changes:
  • Description: (describe the changes proposed in this PR)

@systemcrash
Copy link
Contributor

Please don’t use the words with or without. The reason for gerunds is that the distinct separate actions remain clear because there are multiple actions. With implies "together with". Other languages can have what they like.

@systemcrash
Copy link
Contributor

Otherwise centered is good. Just try to retain the gerunds after "Apply" so it's clear what's about to happen. Do you want the actions after apply to read similarly or something?

* Update help text
* Buttons moved to the center of the overlay
* Button 'Cancle' moved to the right
* Unify button text

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert
Copy link
Member Author

feckert commented Jun 6, 2024

@systemcrash
I hope I have understood your change request correctly.
This is how it would look now.
after

@systemcrash
Copy link
Contributor

systemcrash commented Jun 6, 2024

"Apply, changes now" is odd. Two verbs, or noun verb? The other button is okay, though unreachable alone is tenuous. That button is the first time the GUI is mentioned, and it's not normally obvious that system and GUI are supposed to be client server (and thus distinct and separate).

@systemcrash
Copy link
Contributor

Keep gerunds after Apply.

@jow-
Copy link
Contributor

jow- commented Jun 6, 2024

"Apply changes now" is not entirely correct. Changes are still applied in a delayed manner, the difference is whether they'll get rolled back or not if the browser does not interact with LuCI within the next 90 seconds. Can't suggest something suitable now but the verb should not refer to the "timing" of the change application but to the "stickiness" (for lack of a better word).

@systemcrash
Copy link
Contributor

Yes. So "Apply, committing..." or "Apply, reverting..." pattern encompasses this distinction @jow- mentions.

@jow-
Copy link
Contributor

jow- commented Jun 6, 2024

In the generic uci dialog we're using "Apply unchecked" as button label. Would that work?

@feckert
Copy link
Member Author

feckert commented Jun 6, 2024

@systemcrash
The wording commiting doesn't seem to fit either, see @castillofrancodamian objection from yesterday.
a6f8361#r142789901

In the generic uci dialog we're using "Apply unchecked" as button label. Would that work?

@jow- I like that.

@systemcrash
Copy link
Contributor

Verb + adverb (apply immediately), Verb + adjectival phrase (apply unchecked), verb + gerund (apply, doing x) all work, just a bit differently. Unchecked creates an expectation that all settings actually get checked somehow in another path, in this case the "checked" path. This construction focuses on the first action to the detriment of the successive one. (The gerund construction handles the second one.)

@systemcrash
Copy link
Contributor

"Simplify as much as possible, but not further" 😅

@jow-
Copy link
Contributor

jow- commented Jun 6, 2024

Centering the buttons would make the layout for this modal different compared to any other modal. I'd prefer consistency here.

Maybe it makes sense to introduce new CSS after all, iteratively applying it to other modals too. E.g. change <div class="right"> to <div class="button-row"> and introduce the following new CSS rules:

.modal > .button-row {
  display: flex;
  justify-content: space-between;
}

.modal > .button-row > button:not(:first-of-type) {
  margin-left: .5em;
}

.modal > .button-row > button:last-of-type {
  margin-left: auto;
}

Then reorder the buttons so that Cancel comes last. The CSS rules are generic enough that it should be possible to copy-paste them into every theme. On my device (after dragging the cancel button to the end of the containing div in the inspector), the result looks like this (I didn't change any texts yet):

image

For the sake of consistency I'd prefer if we always put the "no action, abort" button to the lower right corner and the "do something" one(s) to the lower left.

@feckert
Copy link
Member Author

feckert commented Jun 6, 2024

Here are a few suggestions.
3
2
1
4
5
Or right-aligned:
1-right

@jow-
Copy link
Contributor

jow- commented Jun 6, 2024

Actually I had it backwards, "no action, cancel" should be the lower left and "do something" the lower right corner. Pressing Esc in an open modal will also perform a click on the first button (which should then be the non-destructive one).

Considering this, the suggested CSS should be:

.modal .button-row {
  display: flex;
  justify-content: space-between;
}

.modal .button-row > :not(:last-child) {
  margin-right: .5em;
}

.modal .button-row > :first-child {
  margin-right: auto;
}

... with the result looking like this:
image

Here's some examples with the style applied to a few other dialogs:
image

image

image

image

image

@systemcrash
Copy link
Contributor

systemcrash commented Jun 6, 2024

337235382-fc45690b-99e3-4bc6-856d-cdec0c76bc2c

These button titles are the closest to sane and understandable.

The "changes have been made" is not yet concrete (are they or are they not in effect?). Changes are queued up, but are not in effect yet. Thus my changes had "could inhibit" only at the start.

@feckert feckert closed this Jun 7, 2024
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.

3 participants