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

feat: refresh method in themeStore #582

Closed
wants to merge 8 commits into from

Conversation

AntonioVentilii
Copy link
Contributor

@AntonioVentilii AntonioVentilii commented Feb 11, 2025

Motivation

We would like to have a refresh method in the themeStore. It is useful for example, if the theme is changed in the local storage and we need to "refresh" the store: for example, we have the case in OISY where we "set" the theme to SYSTEM, basically deleting the theme in local storage.

Changes

  • Extract an util getCurrentTheme to get the current theme set in the store: this code is already inside initTheme, we just separate it to re-use it.
  • Create sub-function setTheme inside initialTheme that is re-using the code of the select function.
  • Reusing the sub-function above for the method refresh in combination with getCurrentTheme.
  • A bit unrelated (but not that much): we use the initTheme function directly in the initialization of the store, instead of creating a function outside. I think it makes more sense since we do not want a static value when creating this store, but we want to capture the current theme.

Screenshots

Added a few tests for themeStore and initThemeStore.

@AntonioVentilii
Copy link
Contributor Author

@peterpeterparker shall I break this in smaller PRs? If yes, any suggestion?

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

I don't understand your PR — it doesn't seem to implement the function we discussed. I might be missing something, but is it designed this way because some logic remains in OISY? Is that the idea?

To be clear, this is the feature logic I would have expected: #585

@AntonioVentilii
Copy link
Contributor Author

@peterpeterparker Ah! I completely misunderstood then! Yes, the idea was to leave some logic there in OISY, but if that is not the case, for sure let's go with your PR

@AntonioVentilii
Copy link
Contributor Author

As discussed offline, I will close this and continue with the correct implementation in :

@AntonioVentilii AntonioVentilii deleted the feat/reset-method-in-themeStore branch February 12, 2025 08:16
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.

2 participants