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

Disable random article in settings view #955

Merged
merged 1 commit into from
Jul 22, 2023
Merged

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Jul 7, 2023

Fixes #954

@juuz0 juuz0 requested a review from kelson42 July 7, 2023 12:06
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

A more proper fix seems to be the following:

  1. change the type of the parameter of KiwixApp::disableItemsOnLibraryPage(bool libraryDisplayed) and TabBar::libraryPageDisplayed(bool displayed) from bool to a new enum type which tells the kind of the current tab (content, library, settings). Inside that function enable only actions that make sense for the current tab kind.
  2. Naturally after that change KiwixApp::disableItemsOnLibraryPage() and TabBar::libraryPageDisplayed() have to be renamed.

@mgautierfr
Copy link
Member

Agree with @veloman-yunkan, those functions have been created when there was no setting page. Now we have more than 2 kinds of tab, we should create more generic functions to handle "action configuration" when we change tabs.

@juuz0 juuz0 force-pushed the settingsViewRandomIconGrey branch from 29225ac to ce1534b Compare July 14, 2023 13:36
kiwix-desktop didn't acknowledge the different types of tabs available earlier.
These enums provide a way to handle and customize the state of app's different elements based on tab content.
@juuz0 juuz0 force-pushed the settingsViewRandomIconGrey branch from ce1534b to bf4a0ed Compare July 20, 2023 09:30
@kelson42
Copy link
Collaborator

@juuz0 Ready for a new review?

@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 20, 2023

@kelson42 yes

@kelson42 kelson42 merged commit d9d06ea into main Jul 22, 2023
5 checks passed
@kelson42 kelson42 deleted the settingsViewRandomIconGrey branch July 22, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random icon must be greyed if settings open
4 participants