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

WebUI: Restore previously used tab on load #20705

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Piccirello
Copy link
Member

@Piccirello Piccirello commented Apr 15, 2024

This PR restores the users previously used tab (Transfer, Search, RSS, etc.) when the Web UI is reloaded.

Screen.Recording.2024-04-14.at.23.33.19.mov

@Piccirello Piccirello added the WebUI WebUI-related issues/changes label Apr 15, 2024
@Piccirello Piccirello added this to the 5.0 milestone Apr 15, 2024
@Piccirello Piccirello requested a review from a team April 15, 2024 06:35
@Piccirello Piccirello marked this pull request as ready for review April 15, 2024 06:37
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
src/webui/www/private/scripts/client.js Show resolved Hide resolved
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
This simplifies development as developers can now directly set breakpoints in the browser on search code.
The log panel may be displayed before its JS has loaded, which results in an error. This prevents the error by delaying initialization until the necessary scripts are loaded.
Chocobo1
Chocobo1 previously approved these changes May 7, 2024
Comment on lines 1686 to 1687
console.error(`Unexpected 'selected_tab' value: ${previouslyUsedTab}`)
$('transfersTabLink').click();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.error(`Unexpected 'selected_tab' value: ${previouslyUsedTab}`)
$('transfersTabLink').click();
console.error(`Unexpected 'selected_tab' value: ${previouslyUsedTab}`);
$('transfersTabLink').click();
break;

Would be nice to have a break for the last case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a quick search before pushing this change and MDN's examples don't include a break in the default case. It makes sense since there's nothing further that can run.

Copy link
Member

Choose a reason for hiding this comment

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

It is for 'defensive' coding that it will prevent someone adding another case below and forget to add a break up here.
https://softwareengineering.stackexchange.com/questions/201777/break-on-default-case-in-switch

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't make any sense to add a case below the default. This seems like a stylistic choice that is low impact enough to not warrant spending more time on.

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't make any sense to add a case below the default.

You wouldn't believe the kind of stupid mistakes that happens when code changes, especially by naive coders (and even by veteran coders).

This seems like a stylistic choice that is low impact enough to not warrant spending more time on.

I agree about the 'not warrant spending more time' part but not about the 'low impact' part.

Also I would like to reiterate that I'll not be responsible for any breakage of WebUI and I only help review in best effort. So please don't ever ping me about it when it happens. I've done my best (concerning long-term maintainability) but they wouldn't even move their finger for a trivial change. I'll just approve it so everyone is happy.

This logic bug prevented the double click handler from being readded to rows once the search completed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants