Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Add Preferences UI #1538

Merged
merged 2 commits into from
Feb 3, 2021
Merged

Add Preferences UI #1538

merged 2 commits into from
Feb 3, 2021

Conversation

mklanjsek
Copy link
Contributor

What this PR does / why we need it:
This change adds Preferences button at the bottom of the Left Navigation. That will open a Preferences dialog that currently has only two entries: Theme toggle and Navigation toggle. We will add more entries as we go on and merge it with existing Electron prefs after Electron build is fixed.

Screen Shot 2020-10-22 at 12 10 56 PM

Which issue(s) this PR fixes

@scothis
Copy link
Contributor

scothis commented Oct 22, 2020

It would be nice to have an option for the theme to use the OS setting for dark mode

@media (prefers-color-scheme: dark) {
   ...
}

@bryanl
Copy link
Contributor

bryanl commented Oct 23, 2020

There already is an existing preferences component in support of the electron app. Should these two things be merged?

@mklanjsek
Copy link
Contributor Author

@bryanl Yes! Unfortunately Electron is currently broken, so I couldn't merge them, but that will happen next. It shouldn't be hard since I already use your framework and preferences component.

@mklanjsek
Copy link
Contributor Author

@scothis great idea! I made a change so we now use the OS theme by default while still letting the user to override that through Preferences.

@wwitzel3
Copy link
Contributor

I'm hesitant to merge this mainly because the moment we merge it, we add debt to the code base. Since we know this needs to be reconciled with the Electron preferences and those things merged, I think we should hold this PR until we have the Electron build fully functioning again and then rebase this to include the full spectrum of changes.

@wwitzel3 wwitzel3 marked this pull request as draft October 27, 2020 13:49
@mklanjsek
Copy link
Contributor Author

@wwitzel3 I agree, let's have this one marinate a bit longer until Electron issues are sorted out.

@mklanjsek
Copy link
Contributor Author

This is now integrated with Electron providing functionality necessary for 0.17:

  • Added Electron integration,
  • Preferences can now be invoked from app menu,
  • Added Show Labels field to prefs based on design feedback session,
  • Added unit test for preferences service,
  • Minor styles cleanup

After 0.17, we still need to merge client and backend preferences and provide uniform persistence.

@mklanjsek mklanjsek marked this pull request as ready for review January 29, 2021 16:33
@wwitzel3
Copy link
Contributor

I want to deal with that last item in this PR:

  • merge client and backend preferences and provide uniform persistence

This PR was held previously because electron didn't work, but also, and more importantly, I didn't want a divergent path for how we store user preferences.

If we release this and merge the client and backend later, we now have to do some form of migration for those preferences, if we want to be kind to users.

So now we have to consider:

  • know how to migrate 0.17 to 0.18 preference model
  • decide how many versions we want to support the ability to migrate preferences (-2, +1)
  • maintain the preferences migration capability as we expand and add new preferences to later versions.
  • and finally remove old pref migration code

We will want to make a choice about how we store our preferences. Before, we were using internal/electron. We were also using astilectron which we've moved away from (https://github.com/vmware-tanzu/octant/blob/1c57c1ef580f93630e5358814be98872c1f72100/internal/electron/preferences/v1alpha1/util.go)

The question is, do we migrate the Frontend URL Proxy setting up in to this and remove the internal/electron code?
Do we want to leverage the internal/electron data model?

I have opinions, but I'd like to hear from some other folks first.

cc: @GuessWhoSamFoo @mklanjsek

@mklanjsek
Copy link
Contributor Author

Valid points Wayne - I wanted to separate the UI work from persistence and make this PR manageable and easier to review.

It makes sense to address that before the release, but can we open a new issue to track that?

@mklanjsek
Copy link
Contributor Author

Persistence is now covered in #1923

@GuessWhoSamFoo
Copy link
Contributor

Needs a rebase

Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>
Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>
@mklanjsek mklanjsek merged commit 63e2b5a into vmware-archive:master Feb 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Preferences entry to the bottom of Vertical Navigation
5 participants