Skip to content

#1223: use theme service on settings load #1238

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
Merged
merged 4 commits into from
Jul 27, 2022
Merged

#1223: use theme service on settings load #1238

merged 4 commits into from
Jul 27, 2022

Conversation

@davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Jul 21, 2022 •
edited
Loading

Motivation

Incorrect theme shown on first start in settings.

Change description

Use matchMedia method to determine initial UI dropdown value. Patch the app config provider to dispatch based on the OS' theme.

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 21, 2022
davegarthsimpson and others added 2 commits July 22, 2022 09:56
@davegarthsimpson
typo fix
e9c2613
Patched app config to dispatch on OS' theme.
387e52b 
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos
Copy link
Contributor

My latest commit (387e52b) patches the app config provider. The default value from Theia is based on the app config. See here. It always picked up the light theme, now it should dispatch based on the OS' theme.

@kittaakos
Copy link
Contributor

@fstasi, could you please help with the review?
@per1234, could you please verify the changes once more?

Thank you! 🙏

@davegarthsimpson
Copy link
Contributor Author

confirmed this works for me on mac.

@davegarthsimpson davegarthsimpson requested a review from per1234 July 26, 2022 10:05
fstasi
fstasi suggested changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@fstasi fstasi left a comment

Choose a reason for hiding this comment

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

@davegarthsimpson seems good but I think you need to update the PR description

@davegarthsimpson davegarthsimpson requested review from fstasi and removed request for per1234 July 26, 2022 12:11
fstasi
fstasi approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@fstasi fstasi left a comment

Choose a reason for hiding this comment

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

lgtm

@davegarthsimpson davegarthsimpson merged commit f22be3c into main Jul 27, 2022
@davegarthsimpson davegarthsimpson deleted the 1223-incorrect-theme-setting-shown-on-first-start branch July 27, 2022 09:06
@per1234 per1234 added the topic: theme Related to GUI theming label Aug 1, 2022
@per1234 per1234 linked an issue Aug 1, 2022 that may be closed by this pull request
IDE shows correctly picks the theme based on the OS' theme, but it's incorrectly shown in the Settings #1223
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kittaakos kittaakos Awaiting requested review from kittaakos

1 more reviewer

@fstasi fstasi fstasi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic: code Related to content of the project itself topic: theme Related to GUI theming type: imperfection Perceived defect in any part of project

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

IDE shows correctly picks the theme based on the OS' theme, but it's incorrectly shown in the Settings

4 participants

@davegarthsimpson @kittaakos @fstasi @per1234