-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Desktop: Replace the edit profile config menu option with a gui to manage profiles #13771
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this! I've left a few comments based on an initial review of the code.
Edit: The new ProfileManagementScreen code is very similar to the existing resource/attachment management screen. A refactor could make sense here to reduce code duplication.
| // eslint-disable-next-line @typescript-eslint/ban-types -- Changing types for these variables would be too big of a refactoring | ||
| dispatch: Function; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // eslint-disable-next-line @typescript-eslint/ban-types -- Changing types for these variables would be too big of a refactoring | |
| dispatch: Function; | |
| dispatch: Dispatch; |
Types: Dispatch can be imported from 'redux';
| ); | ||
| }; | ||
|
|
||
| class ProfileManagementScreenComponent extends React.Component<Props, State> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Please use function components for new code.
| const style = this.props.style; | ||
| const theme = themeStyle(this.props.themeId); | ||
|
|
||
| const rootStyle: CSSProperties = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@personalizedrefrigerator Using scss for styling looks to be problematic with regards to theme based styling. The themeToCss function will allow you to get top level styles in variables from theme.ts eg. using --joplin-font-family, to access those values in scss. But how would you get a nested style such as theme.textStyle or theme.buttonStyle? You could use both 'className' and 'style' on an element, but the problem is that 'style' would take precedence as it will be inline, so anything you put in the scss would have lower priority unless you use !important, which feels like bad practice. I could get around this by duplicating the styling in theme.ts / globalStyle.ts in my scss file, but that feels counter productive and would make using scss a drawback rather than improving maintainability. How would you recommend to deal with this? |
|
@personalizedrefrigerator Ok, so I have actioned your last 3 comments. I implemented the styling with scss as best as I could, but I had to mix and match with global inline styling as well, in order to avoid code duplication and get the dynamic styling (such as the container height) to work. I turns out it was not necessary to include the theme based styling for the parts which I needed to add which would conflict, but I do think some more guidance should be added on the https://joplinapp.org/help/dev/spec/desktop_styling/ page for how to deal with this scenario. Also I had to use lower camel case for 'single word' class names, in order to avoid linting issues with the spell check where it is actually 2 words combined. With the new changes, the layout looks equivalent to the original and I have verified scrolling and resizing all work correctly. I tested the screen with the dark theme as well and verified the styling matches how other screens look with the dark theme.
Regarding this, I'm not sure combining the screen logic with the attachment screen is a good idea in this case. To do that would require extracting the column definitions, the data, the buttons and their behaviour out of the component to do that, which is quite a lot of uncommon logic to be honest. Additionally, in order to navigate to the ProfileManagementScreen, the screen is created via the Root.tsx render method, which I don't think will allow passing properties into the ProfileManagementScreen at current, so it could require a lot of refactoring in order to make it possible to re-use the base component. |
Main goal: Address [this comment](laurent22#13771 (comment)) by @mrjo118 by adding utilities that allow extending base styles. See also: [RSCSS: Simplifying nested components](https://ricostacruz.com/rscss/nested-components.html).
| import * as exportFolders from './exportFolders'; | ||
| import * as exportNotes from './exportNotes'; | ||
| import * as focusElement from './focusElement'; | ||
| import * as manageProfiles from './manageProfiles'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to keep in mind is that commands are user-facing and may be used in plugins and some on keyboard shortcuts. It's unlikely this one is used anywhere but as a general rule we should be careful, unless we have a very good reason to break backward compatibility.
We can always make a shortcut from the old command to the new one, however in this case I don't see a strong reason to change the name. manageProfiles is ok I guess, and so is editProfileConfig. If anything editSomething is common throughout the app while there's no instance of manageSomething so I would favor the existing command.
Or maybe we keep editProfileConfig and add a new one but I think it should be called something like showProfileEditor, like on mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now re-instated the editProfileConfig command and just not used it anywhere in the UI. I've also renamed the manageProfiles command to be showProfileEditor
| @@ -0,0 +1,210 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be called ProfileEditor like on mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| await saveProfileConfig(`${Setting.value('rootProfileDir')}/profiles.json`, newProfileConfig); | ||
| dispatch({ | ||
| type: 'PROFILE_CONFIG_SET', | ||
| value: newProfileConfig, | ||
| }); | ||
| } catch (error) { | ||
| logger.error(error); | ||
| bridge().showErrorMessageBox(error.message); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor to avoid duplicating the code - maybe move all this to a function and call it from both here and onProfileDelete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
As #13804 is suggesting changes to the resources screen which this is based on, the layout is diverging. The search is not really necessary for this screen, as you would not expect a user to have an excessive amount of profiles, so I have just removed it to reduce the maintenance impact. See screenshot: |
Currently on the desktop app, it is possible to create new profiles and switch between them via menu options. However, in order to rename or delete profiles, unlike the mobile app (which has a manage profiles screen), this can only be done by editing the profiles.json file which can be accessed via the edit profile config menu option, and manually deleting the matching directory in the case of deletion. This is not user friendly for non technical users, and could lead to them messing up their Joplin profiles, or even deleting the wrong profile.
This PR replaces the edit profile config menu option with a manage profiles option, which provides a GUI for renaming and deleting profiles. This GUI is based on the note attachments management screen.
Testing
I have tested the change via the UI. Things which were tested:
See video:
Please note, the search bar has since been removed.
electron_JKokiZKFTb.mp4