Skip to content

Web UI: Inject a Go Back link on all pages, except the index and upload page#1302

Merged
rdmark merged 5 commits intodevelopfrom
rdmark-issue-1301
Nov 8, 2023
Merged

Web UI: Inject a Go Back link on all pages, except the index and upload page#1302
rdmark merged 5 commits intodevelopfrom
rdmark-issue-1301

Conversation

@rdmark
Copy link
Copy Markdown
Member

@rdmark rdmark commented Nov 4, 2023 •
edited
Loading

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

@rdmark rdmark requested a review from nucleogenic November 4, 2023 12:52
@rdmark rdmark mentioned this pull request Nov 4, 2023
Closed
@rdmark rdmark force-pushed the rdmark-issue-1301 branch from b85cd1a to ecd6160 Compare November 4, 2023 13:02
@rdmark rdmark marked this pull request as draft November 4, 2023 13:16
@rdmark rdmark force-pushed the rdmark-issue-1301 branch from ecd6160 to 3c3c94b Compare November 4, 2023 13:19
@rdmark rdmark marked this pull request as ready for review November 4, 2023 13:19
@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 4, 2023

@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!

@uweseimet
Copy link
Copy Markdown
Contributor

uweseimet commented Nov 4, 2023

IMHO it is not good practice to add new features just before a release, instead of letting the release stabilize and just fix bugs.

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 4, 2023

@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.

@uweseimet
Copy link
Copy Markdown
Contributor

uweseimet commented Nov 4, 2023

@rdmark No need to postpone, but I am just a bit worried about all these changes when a release is close.

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 4, 2023

Agreed. Avoiding scope creep near a release is important. My aim is to fix only bugs that are revealed by testing.

@erichelgeson erichelgeson removed their request for review November 4, 2023 16:07
nucleogenic
nucleogenic reviewed Nov 4, 2023
View reviewed changes
@rdmark rdmark force-pushed the rdmark-issue-1301 branch 2 times, most recently from 71277c2 to 77f0ac5 Compare November 8, 2023 03:26
@rdmark rdmark requested a review from nucleogenic November 8, 2023 03:27
@rdmark rdmark force-pushed the rdmark-issue-1301 branch from 77f0ac5 to 630678c Compare November 8, 2023 03:44
@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 8, 2023

@nucleogenic I reworked this PR after your feedback. Please review when you have a moment to spare!

@benjamink
Copy link
Copy Markdown
Collaborator

benjamink commented Nov 8, 2023

Would a "house" icon make more sense than the << icon? << makes me think "back", not "home". Otherwise it looks good to me & works as expected.

nucleogenic
nucleogenic approved these changes Nov 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@nucleogenic nucleogenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 8, 2023

@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.

@rdmark rdmark changed the title Web UI: Inject a Go Back link on all pages, except the index page Web UI: Inject a Go Back link on all pages, except the index and upload page Nov 8, 2023
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Nov 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
30.9% 30.9% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@rdmark rdmark merged commit 76b0f43 into develop Nov 8, 2023
@rdmark rdmark deleted the rdmark-issue-1301 branch November 8, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nucleogenic nucleogenic nucleogenic approved these changes

@akuker akuker Awaiting requested review from akuker akuker is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rdmark @uweseimet @benjamink @nucleogenic