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

treewide: add new css class button-row #7153

Merged
merged 20 commits into from
Jul 17, 2024

Conversation

feckert
Copy link
Member

@feckert feckert commented Jun 7, 2024

Base on the discussion with @systemcrash @jow- on PR #7152, I have added the suggested css class button-row.

New view for the connectivity change that I originally wanted to change.
after

I have tested the changes for the themes luci-theme-material and luci-theme-bootstrap.

  • Interface stop
  • OPKG update/feeds
  • Upload

When comparing the Save & Apply I noticed a problem that looks right with luci-theme-material but not so right with luci-theme-bootstrap.

luci-theme-bootstrap (NOK):
bootstrap

luci-theme-material (OK):
material

I think the problem is with the CSS! Unfortunately I have no idea how to solve this.

  • 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)

feckert added 12 commits June 7, 2024 13:33
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
This is a preparation commit so the buttons could get the new css class
'button-row'

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
This change is required, so that the cancel button is the first and thus
closes the modal when the 'ESC' button is pressed.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert feckert requested a review from jow- June 7, 2024 12:03
@systemcrash
Copy link
Contributor

That blue save and apply button i the OK example is virtually unreadable

@dannil
Copy link
Contributor

dannil commented Jun 7, 2024

That blue save and apply button i the OK example is virtually unreadable

This isn't a bug in this PR tho, it's been that for a long time in the Material theme (I run it as default) so doesn't need to be addressed in this PR in my mind.

I think the problem is with the CSS! Unfortunately I have no idea how to solve this.

It's because all list (li) elements inside the .modal class is styled as color: gray and the buttons in the Configuration / Changes dialog is wrapped as list items, which is a side-effect of trying to color something else in the modals, I haven't figured out what yet tho.

@systemcrash
Copy link
Contributor

Unreadability (illegibility) bears pointing out and... if possible, fixing. I don't use that theme so I've never noticed it. 🙃

@systemcrash
Copy link
Contributor

If you figure it out, pepper in some code comments to an obnoxious degree so others tweaking css benefit...

@dannil
Copy link
Contributor

dannil commented Jun 7, 2024

Unreadability (illegibility) bears pointing out and... if possible, fixing. I don't use that theme so I've never noticed it. 🙃

Absolutely, if it's easy to fix we should definitely do it in this PR if possible. I looked into this a while ago as part of another thing, and then I didn't find what the styling could be used for, so it may be hard to verify if there will be any regressions worse than the button illegibility by just removing it.

@systemcrash
Copy link
Contributor

Just remove and break stuff then test. Should be OK ™

@feckert
Copy link
Member Author

feckert commented Jun 12, 2024

@systemcrash @dannil
I am not a CSS expert. So I'm not able to fix this without delving deeper into CSS, which I want to avoid.
Any help to fix this in CSS will be appreciated

@systemcrash
Copy link
Contributor

@feckert So your wording is still problematic (in English), although we could get away with:

Apply, reverting if connectivity is lost
Apply, reverting in case of connectivity loss

As you might guess, I want to avoid after here because after makes it seem like an implied consequence of this action, i.e. like it (connectivity loss) should happen.
Apply, reverting after connectivity loss
Apply, but revert after connectivity loss

@feckert feckert force-pushed the pr/20240607-luci-base branch 2 times, most recently from 04a49a7 to 82f4b3c Compare July 9, 2024 11:58
@feckert
Copy link
Member Author

feckert commented Jul 9, 2024

@systemcrash thanks for the suggested wording. I have update the pullrequest.

@feckert feckert marked this pull request as ready for review July 9, 2024 12:00
@systemcrash
Copy link
Contributor

Np.

Did one of the buttons function change?

@feckert
Copy link
Member Author

feckert commented Jul 9, 2024

@systemcrash Thank you for seeing this. I made a mistake when rebasing.
It should be OK now.

@systemcrash
Copy link
Contributor

Ok. Better. I think that the last button would probably do well called "Apply unchecked", as a suggestion - this wording has enough contrast with the middle button to make its purpose distinct.

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

feckert commented Jul 9, 2024

@systemcrash done

@systemcrash
Copy link
Contributor

Looks good to me. Merge at your leisure.

@feckert feckert merged commit a0dcba5 into openwrt:master Jul 17, 2024
5 checks passed
@feckert feckert deleted the pr/20240607-luci-base branch July 17, 2024 07:31
@zdz2019
Copy link

zdz2019 commented Jul 17, 2024

This change will cause luci-proto-wireguard to break.

01
02

@feckert
Copy link
Member Author

feckert commented Jul 17, 2024

Will have look. Thanks for feedback

@dannil
Copy link
Contributor

dannil commented Jul 17, 2024

There seems to be a regression in the state change on the "Upload" button in "Flash new firmware image", it doesn't toggle to the "active state" when you upload a valid sysupgrade. I haven't investigated if it's a general issue. It's still possible to click it tho to proceed so it's a styling thing.

Before:
Screenshot_3

After:
Screenshot_4

@systemcrash
Copy link
Contributor

This change will cause luci-proto-wireguard to break.

@feckert I went back and double-checked the latest wireguard changes, and buttons there still work fine, so it's related to changes from this PR. Some of the buttons in wg might lack some essential styling, perhaps?

@feckert
Copy link
Member Author

feckert commented Jul 18, 2024

I have managed to display the StackedSubModul of the wireguard peers again by adjusting the querySelector (#7200).
But now there is another problem with the Back button of StackedSubModul hanlding of the wireguard peer page. It could be closed but the previous SubModul close button does not! I have to reload the whole page.

The error must be somewhere here!
https://github.com/openwrt/luci/blob/master/modules/luci-base/htdocs/luci-static/resources/form.js#L3204-L3217

I broke it, but @jow- can you maybe help here?

@systemcrash
Copy link
Contributor

Maybe revert for now, and queue up the cumulative relevant changes into the new PR 7200

@feckert
Copy link
Member Author

feckert commented Jul 19, 2024

I have update the PR #7200, that should fix the regression for sub modal handling that is used in the wireguard proto.
@zdz2019 please try

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.

5 participants