luci icon indicating copy to clipboard operation
luci copied to clipboard

treewide: add new css class button-row

Open feckert opened this issue 1 year ago • 7 comments

Base on the discussion with @systemcrash @jow- on PR https://github.com/openwrt/luci/pull/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.

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

feckert avatar Jun 07 '24 12:06 feckert

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

systemcrash avatar Jun 07 '24 18:06 systemcrash

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.

dannil avatar Jun 07 '24 20:06 dannil

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

systemcrash avatar Jun 07 '24 20:06 systemcrash

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

systemcrash avatar Jun 07 '24 20:06 systemcrash

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.

dannil avatar Jun 07 '24 20:06 dannil

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

systemcrash avatar Jun 07 '24 20:06 systemcrash

@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

feckert avatar Jun 12 '24 06:06 feckert

@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

systemcrash avatar Jul 08 '24 14:07 systemcrash

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

feckert avatar Jul 09 '24 12:07 feckert

Np.

Did one of the buttons function change?

systemcrash avatar Jul 09 '24 12:07 systemcrash

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

feckert avatar Jul 09 '24 13:07 feckert

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.

systemcrash avatar Jul 09 '24 13:07 systemcrash

@systemcrash done

feckert avatar Jul 09 '24 14:07 feckert

Looks good to me. Merge at your leisure.

systemcrash avatar Jul 09 '24 14:07 systemcrash

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

01 02

zdz2019 avatar Jul 17 '24 12:07 zdz2019

Will have look. Thanks for feedback

feckert avatar Jul 17 '24 12:07 feckert

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

dannil avatar Jul 17 '24 17:07 dannil

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?

systemcrash avatar Jul 17 '24 22:07 systemcrash

I have managed to display the StackedSubModul of the wireguard peers again by adjusting the querySelector (https://github.com/openwrt/luci/pull/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?

feckert avatar Jul 18 '24 08:07 feckert

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

systemcrash avatar Jul 18 '24 13:07 systemcrash

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

feckert avatar Jul 19 '24 06:07 feckert