Skip to content

ui: apply button#10134

Open
sopex wants to merge 1 commit intoopnsense:masterfrom
sopex:apply
Open

ui: apply button#10134
sopex wants to merge 1 commit intoopnsense:masterfrom
sopex:apply

Conversation

@sopex
Copy link
Copy Markdown
Member

@sopex sopex commented Apr 12, 2026

Hides apply buttons that leak to subtabs and small UI fixes around these buttons to make the UI more homogenous.

@sopex sopex changed the title ui: apply button improvements ui: apply button Apr 12, 2026
@fichtner fichtner self-assigned this Apr 12, 2026
@fichtner
Copy link
Copy Markdown
Member

fichtner commented Apr 12, 2026

This will break with #10103 and I think if we want this we need to make it generic. Similar concern was voiced before but in general I’m not sure if it’s worth the trouble implementing. Hiding could always introduce subtle bugs that make the apply disappear more permanently when targeted.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Apr 12, 2026

This will break with #10103 and I think if we want this we need to make it generic. Similar concern was voiced before but in general I’m not sure if it’s worth the trouble implementing. Hiding could always introduce subtle bugs that make the apply disappear more permanently when targeted.

I think it's worth adding an id to #10103, people don' t like buttons that do nothing :)

@fichtner
Copy link
Copy Markdown
Member

a class would be more flexible, but yes

@Monviech
Copy link
Copy Markdown
Member

It's important (for me) that the Apply button is always visible and clickable. I don't want hiding or disabling logic.

Apply can be necessary even outside of config changes to regenerate templates and reload the service (the service buttons dont exist on all pages and what they do is more opaque imo).

I spent quite some effort to make sure the button is at the same spot everywhere and behaves the same, I don't want to shoot this down at the moment.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Apr 12, 2026

It's important (for me) that the Apply button is always visible and clickable. I don't want hiding or disabling logic.

Apply can be necessary even outside of config changes to regenerate templates and reload the service (the service buttons dont exist on all pages and what they do is more opaque imo).

I spent quite some effort to make sure the button is at the same spot everywhere and behaves the same, I don't want to shoot this down at the moment.

Everyone is working on Sunday :(

I am not sure why we are not trusting the MVC model, the whole page is just a show/hide div.
Plus, 3/4 pages have a restart button.

Maybe only IMO but this just seems like broken UI, not the thoughtful Apply button I am sure you wanted to add:

image

@Monviech
Copy link
Copy Markdown
Member

It's just a slippery slope, I want the apply button to be as dumb and predictable as possible.

When we start to hook it conditionally again it's going to be a slippery slope (next would be why not hide it on more pages, eg when config is not dirty etc...), even if the intentions are good.

Just wanted to add my two cents without blocking anything.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Apr 12, 2026

I understand what you are saying, I agree with you, and I like that the apply button is in its predictable place with its nice box.

I just believe some cases, like these, and like the "Log file" pages of the services (where apply does not exist). Don't need it, and its presence becomes confusing.
Maybe this is where the line should be drawn on not needed + visually confusing, otherwise uniformity needs to be kept.

Obviously, fair enough to have an opinion, you have done way too much work to stay idle :)

@Monviech
Copy link
Copy Markdown
Member

If its pages with two tabs just put the apply button template into the scope of the tab it should show. I think I've already done it like this in some spots. Its a one line pragmatic code change with no side effects.

@sopex
Copy link
Copy Markdown
Member Author

sopex commented Apr 12, 2026

If its pages with two tabs just put the apply button template into the scope of the tab it should show. I think I've already done it like this in some spots. Its a one line pragmatic code change with no side effects.

I tried that, it kills your nice detached box, and it's like appending it to the existing box.
Plus, there are 3 tab pages.
I know I am not helping :)

@Monviech
Copy link
Copy Markdown
Member

Monviech commented Apr 12, 2026

I was under the assumption it worked when done correctly, just dropping the apply button template into the div of the tab.

Also the apply button template lets you define button IDs, so you can reuse it in multiple tabs without any overlap.

I can give this a look at some point if there are challenged here. I think just dropping the template in where it should be shown is more pragmatic than JS show/hide from a maintainability standpoint, its more declarative.

@fichtner
Copy link
Copy Markdown
Member

Thinking about this more and looking the diff it would be more beneficial to fully unify apply button use first before trying to micro optimise single pages for cosmetic preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants