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

Add Font preferences #437

Closed
wants to merge 7 commits into from
Closed

Add Font preferences #437

wants to merge 7 commits into from

Conversation

jrysana
Copy link
Contributor

@jrysana jrysana commented Oct 22, 2021

Per user request, adds preferences to modify font-size for logs and the rest of the UI, as well as ligatures settings.

  • Control CSS with pref variables
  • Bind Preferences page controls with localStorage
  • Use dropdown lists and number inputs for unit-based sizing controls and list selection ligature controls
  • Fix unit input loading bug (works fine isolated, loads empty from preferences object)

image
image

@jrysana jrysana marked this pull request as ready for review October 25, 2021 00:11

const key = 'io.deref.exo/preferences';

type Preferences = Record<string, string>;
Copy link
Member

Choose a reason for hiding this comment

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

You might want to spell out the keys explicitly:

interface Preferences {
  'main-font-size': string;
  ...
}

gui/src/pages/Preferences.svelte Outdated Show resolved Hide resolved
gui/src/pages/Preferences.svelte Outdated Show resolved Hide resolved
on:click={() => {
theme.apply('auto');
preferences.reset();
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the timeout here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would seem as though there is some sort of hidden race condition here, where the form's dirtyPrefs are set to the non-reset version of $preferences before the reset actually happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternate solution would've been to just reload the window after a reset.

Copy link
Member

Choose a reason for hiding this comment

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

Solving a race condition by adding a timeout is almost never the right solution :-P

We should understand what is going wrong. I suspect that this code can be simplified a fair bit with the appropriate Svelte idioms.


type Preferences = Record<string, string>;

let dirtyPrefs: Preferences = {};
Copy link
Member

Choose a reason for hiding this comment

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

If you make Preferences an interface with explicit keys, then this may have to become Partial<Preferences>

gui/src/pages/Preferences.svelte Outdated Show resolved Hide resolved
gui/src/pages/Preferences.svelte Show resolved Hide resolved
gui/src/lib/preferences.ts Show resolved Hide resolved
@jrysana
Copy link
Contributor Author

jrysana commented Oct 25, 2021

Thanks for the review @brandonbloom I'll address those comments and add in improved dynamic form controls for unit-suffixed numeric preferences as well as list selection inputs.

@jrysana jrysana mentioned this pull request Oct 26, 2021
@brandonbloom
Copy link
Member

Closing this for now. There are merge conflicts, incomplete bits, and a known bug. Rather than fixit up, it's small enough that we can re-create this after we figure out how configuration story, which might have some impact on the preferences story.

@brandonbloom brandonbloom deleted the add-font-preferences branch May 14, 2022 00:49
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