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.

Sign up for GitHub

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

Jump to bottom

Command Palette: Maintain page state when changing theme #59787

Merged
merged 2 commits into from Dec 5, 2022
Merged

Command Palette: Maintain page state when changing theme #59787

merged 2 commits into from Dec 5, 2022

Conversation

ashharrison90
Copy link
Contributor

What is this feature?

  • stops doing a full page reload when changing the theme

Why do we need this feature?

  • allows us to maintain state in e.g. Explore

Who is this feature for?

  • everyone! 🙌

Which issue(s) does this PR fix?:

Fixes #56840

Special notes for your reviewer:

@ashharrison90 ashharrison90 added this to the 9.3.2 milestone Dec 5, 2022
@ashharrison90 ashharrison90 requested review from ifrost, gelicia, L-M-K-B and a team December 5, 2022 10:38
@ashharrison90 ashharrison90 self-assigned this Dec 5, 2022
@ashharrison90 ashharrison90 requested review from joshhunt and yaelleC and removed request for a team December 5, 2022 10:38
gelicia
gelicia reviewed Dec 5, 2022
View reviewed changes
public/app/core/services/toggleTheme.ts
@@ -7,11 +7,10 @@ import { contextSrv } from '../core';

import { PreferencesService } from './PreferencesService';

export async function toggleTheme(runtimeOnly: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to rename this file too?

gelicia
gelicia approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@gelicia gelicia left a comment

Choose a reason for hiding this comment

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

Looks awesome! It looks like this approach avoids the bugs you see when toggling themes with keybindings (c t, c r)as well.

@ashharrison90
Copy link
Contributor Author

@gelicia it uses exactly the same function as the c t keybindings now (so should have the same bugs 😅 )... but we did just recently fix some bugs with c t here so maybe that's why they're fixed?

@gelicia
Copy link
Contributor

gelicia commented Dec 5, 2022

@gelicia it uses exactly the same function as the c t keybindings now (so should have the same bugs 😅 )... but we did just recently fix some bugs with c t here so maybe that's why they're fixed?

Hmm yeah it is using the same code but I still see the partial theme on the home screen using the keybindings and not the command palette.
Screen Shot 2022-12-05 at 8 07 50 AM

@ashharrison90
Copy link
Contributor Author

@gelicia is that using c r instead of c t? 🤔

command palette and c t should behave exactly the same. c r is a little weird because it doesn't make any backend call, so you end up with this half state... that's part of the reason why we haven't documented it anywhere as well 😬

@gelicia
Copy link
Contributor

gelicia commented Dec 5, 2022

c r is a little weird because it doesn't make any backend call, so you end up with this half state... that's part of the reason why we haven't documented it anywhere as well 😬

Ah got it, didn't realize there was a distinction between the keybindings. Thanks!

@ashharrison90 ashharrison90 merged commit 46adfb5 into main Dec 5, 2022
@ashharrison90 ashharrison90 deleted the ash/56840 branch December 5, 2022 16:12
@grafanabot
Copy link
Contributor

The backport to v9.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-59787-to-v9.3.x origin/v9.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 46adfb596ddb09e6d003e4845eb4a3be6473f8f6
# Push it to GitHub
git push --set-upstream origin backport-59787-to-v9.3.x
git switch main
# Remove the local backport branch
git branch -D backport-59787-to-v9.3.x

Then, create a pull request where the base branch is v9.3.x and the compare/head branch is backport-59787-to-v9.3.x.

@grafanabot grafanabot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label Dec 5, 2022
ashharrison90 added a commit that referenced this pull request Dec 5, 2022
@ashharrison90
Command Palette: Maintain page state when changing theme (#59787)
d3e8c09 
* use same function as the change theme keybindings

* rename toggleTheme service to just theme

(cherry picked from commit 46adfb5)
@ashharrison90 ashharrison90 mentioned this pull request Dec 5, 2022
Merged
ashharrison90 added a commit that referenced this pull request Dec 12, 2022
@ashharrison90
Command Palette: Maintain page state when changing theme (#59787) (#5…
d370327 
…9841)

* use same function as the change theme keybindings

* rename toggleTheme service to just theme

(cherry picked from commit 46adfb5)
@gelicia gelicia mentioned this pull request Dec 15, 2022
Closed
GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
@ashharrison90 @GuaYounesPW
Command Palette: Maintain page state when changing theme (grafana#59787…
544b89f 
…) (grafana#59841)

* use same function as the change theme keybindings

* rename toggleTheme service to just theme

(cherry picked from commit 46adfb5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gelicia gelicia gelicia approved these changes

@ifrost ifrost Awaiting requested review from ifrost

@L-M-K-B L-M-K-B Awaiting requested review from L-M-K-B

@joshhunt joshhunt Awaiting requested review from joshhunt joshhunt is a code owner automatically assigned from grafana/grafana-frontend-platform

@yaelleC yaelleC Awaiting requested review from yaelleC yaelleC is a code owner automatically assigned from grafana/grafana-frontend-platform

Assignees

@ashharrison90 ashharrison90

Labels
add to changelog area/frontend area/ui/theme backport v9.3.x backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. enterprise-ok type/bug
Projects
Grafana Frontend Platform
Archived in project
Milestone
9.3.2
Development

Successfully merging this pull request may close these issues.

Command Palette: Explore state is reset after theme changing
3 participants
@ashharrison90 @gelicia @grafanabot