-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Web UI: Inject a Go Back link on all pages, except the index and upload page #1302
Conversation
rdmark
commented
Nov 4, 2023
•
edited
Loading
edited
- Removes the repeated "Go to Home" link on every subpage
- Adds a "Go to Home" link to the base template, with a conditional that excludes it from the index page
- Move the Manual link into the footer, and remove the now-unused icon
- Adds log controls to the Logging page, removing the need to have a Go Back link in the first place (introduces mild code duplication, but I think it's acceptable)
- Move inline style to stylesheet (Sonar code smell)
b85cd1a
to
ecd6160
Compare
ecd6160
to
3c3c94b
Compare
@nucleogenic What do you think about this method of going back in browser history? I have a vague memory that this is "vintage friendly", i.e. compatible with very old browsers. Other suggestions much welcome! |
IMHO it is not good practice to add new features just before a release, instead of letting the release stabilize and just fix bugs. |
@uweseimet The argument for making this change now, is that with the introduction of the Settings page, the navigation away from the Logging page got awkward. Before, the Go to Home link would take you back to the page where you could initiate another fetching of logs, but this is no longer the case. It's not a critical issue of course, since the browser back button is a good workaround for the same. Just might be initially cumbersome for users. I'm ok to postpone the fix. |
@rdmark No need to postpone, but I am just a bit worried about all these changes when a release is close. |
Agreed. Avoiding scope creep near a release is important. My aim is to fix only bugs that are revealed by testing. |
71277c2
to
77f0ac5
Compare
77f0ac5
to
630678c
Compare
@nucleogenic I reworked this PR after your feedback. Please review when you have a moment to spare! |
Would a "house" icon make more sense than the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@benjamink Made it a house icon! @nucleogenic Yay! Sorry again for the aggressive force pushing. I got used to it in another project that is strict about always having a "clean" changeset in active PRs. ;) I pushed a small change to tweak the Classic theme, and the device info table. |
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |