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

Desktop: Allow for custom Joplin theme and Ace editor styles #2099

Merged
merged 14 commits into from
Dec 13, 2019

Conversation

devonzuegel
Copy link
Contributor

@devonzuegel devonzuegel commented Nov 17, 2019

Discussed in https://discourse.joplinapp.org/t/custom-stylessheets/384/12?u=devonzuegel

This allows users to define custom Ace editor styles in editor.css, similar to how they can define custom styles for the rendered Markdown pane in userstyle.css.

For example, I've added this to my ~/.config/joplindev-desktop/editor.css:

image

... which in this case italicizes and bolds the emphasis tokens in the Ace editor and puts an ugly orange background in place of the default blue sidebar color:

image

I'd be curious to get @laurent22's perspective though before I go further with this. Three things I'd want to improve if you are interested in this general concept before this is a real PR ready for review:

  1. [FIXED] The naming I went with is not very elegant alongside the userstyle.css file. If I were starting from scratch I'd make them something like joplin-theme.css and rendered-markdown.css to make it more clear to users which is which. It might be too late to do that though since userstyle.css has been in use for a while, and there may be an even better option.

  2. It would be ideal if users could update both css files from inside the Preferences tabs, rather than having to dig around ~/.config/joplin-desktop/. Ideally this would be direct editing from inside there for minimum navigation required, but an incremental step towards this would be to add a link from in there that when clicked opens up your default text editor to that file + a quick description of what adding to each file does.

    Done! Here's how it looks:

  3. [FIXED] Right now, await this.loadCustomAceStyles is called on every ACTION, rather than handled properly by the reducer. This means the styles are reloaded on every action, which is not good. I'll fix this of course.

@devonzuegel devonzuegel changed the title Desktop: Allow for custom Ace editor styles Desktop: Allow for custom Joplin theme and Ace editor styles Nov 17, 2019
@laurent22
Copy link
Owner

Thanks @devonzuegel, I think that would be a useful change and I'd be glad to merge it when it's ready.

To load the CSS file, I think a good place would be the start() method of BaseApplication as that way we're sure it's only loaded once. Later if we want something more advanced, we can watch the CSS file for changes and reload it as needed.

Naming is a bit tricky indeed since userstyle.css is already taken. To be more consistent, something like Firefox would be good: They have userContent.css for the page, and userChrome.css for the UI. For now, maybe let's call the new CSS file "userchrome.css" since it's more or less consistent with "userstyle.css"?

Comment on lines -1 to -9
const layoutUtils = {};

layoutUtils.size = function(preferred, min, max) {
if (preferred < min) return min;
if (typeof max !== 'undefined' && preferred > max) return max;
return preferred;
};

module.exports = layoutUtils;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this file isn't used anywhere in the codebase, so I deleted it. Lmk if I missed something with my global search though!

@tessus tessus added desktop All desktop platforms editor labels Nov 26, 2019
@devonzuegel devonzuegel marked this pull request as ready for review November 27, 2019 21:47
@laurent22
Copy link
Owner

Thanks for the update @devonzuegel! I didn't go through all the diffs yet but will do soon.

@tessus
Copy link
Collaborator

tessus commented Nov 29, 2019

I like the new change, but I'm a bit puzzled by the naming:

  • Custom stylesheet for rendered markdown -> userchrome.css
  • Custom stylesheet for Joplin-wide app styles -> userstyle.css

Until now, we only used userstyle.css which basically allowed us to change how the rendered markdown will look like. But it now looks like the userstyle.css is for something else and userchrome.css is supposed to be used to change the rendered markdown.

Maybe I'm missing something.

Update: I think it should rather be ike this, unless I completely misunderstood the reason for this PR:

  • Custom stylesheet for markdown tokens (editor) -> userchrome.css
  • Custom stylesheet for rendered markdown (preview) -> userstyle.css

@devonzuegel
Copy link
Contributor Author

devonzuegel commented Dec 1, 2019

I'm a bit puzzled by the naming:

  • Custom stylesheet for rendered markdown -> userchrome.css
  • Custom stylesheet for Joplin-wide app styles -> userstyle.css

You're right, I accidentally swapped the names! Fixed. Thank you for catching, that would've been super confusing to users.

Update: I think it should rather be ike this, unless I completely misunderstood the reason for this PR:

  • Custom stylesheet for markdown tokens (editor) -> userchrome.css
  • Custom stylesheet for rendered markdown (preview) -> userstyle.css

The reason for the PR was partially to support markdown tokens, but the purpose is broader than just that—the Joplin-wide styles support customization of the whole app via CSS file, including but not limited to the markdown tokens. So for example, you could change the sidebar color with this if you'd like too, which is lighter weight than creating a full theme change.

@devonzuegel
Copy link
Contributor Author

All righty @laurent22, I incorporated your comments. I think this is ready for final review. Thank you!

@eschafer
Copy link

eschafer commented Dec 9, 2019

I'm really excited for this to go through - thanks for working on it!

The only thing I noticed was that nothing happens when you click on the preference buttons if you don't already have style sheets in the config folder. It'd be nice if those files could be created automatically, but if not, some sort of messaging might help.

Actually one more thing that might be out of scope: it'd be nice if the dev tools showed the contents of the whole site rather than just the note preview. Without that, I have to build the app myself or look through github to figure out which selectors I can use.

@devonzuegel
Copy link
Contributor Author

It'd be nice if those files could be created automatically, but if not, some sort of messaging might help.

Great idea @eschafer! Updated the PR so that now it creates the file before opening it if it's not there.

Actually one more thing that might be out of scope: it'd be nice if the dev tools showed the contents of the whole site rather than just the note preview. Without that, I have to build the app myself or look through github to figure out which selectors I can use.

If you run the local build you can get two Developer Tools windows, one for the rendered markdown (like in the normal pre-built app, which is styled by userstyle.css) and one for the rest of the app (styled by userchrome.css, which isn't usually available in the pre-built app for some reason).

Here's how it looks (right = rendered Markdown dev tools, bottom = dev tools for the rest of the app):

image


One issue with the userstyle.css and userchrome.css naming is that by default they open with Firefox (or when I removed Firefox, Safari). This isn't great because they're not editable directly from there. I'd prefer they open in my text editor, so there's no additional step to open them somewhere editable. Any thoughts on this @laurent22? I don't think this is blocking for this PR, but not optimal.

@laurent22
Copy link
Owner

One issue with the userstyle.css and userchrome.css naming is that by default they open with Firefox (or when I removed Firefox, Safari). This isn't great because they're not editable directly from there. I'd prefer they open in my text editor, so there's no additional step to open them somewhere editable. Any thoughts on this @laurent22? I don't think this is blocking for this PR, but not optimal.

I think this is fine for now but indeed it could be improved. Perhaps we could use the configured external editor to open these CSS files?

Another issue was the header on Setting.js - I've left a comment which you might have missed, so for now I've removed the header, and we can maybe look at improving the situation with the lib folder in a different PR.

Otherwise it's all good and ready to merge!

@laurent22 laurent22 merged commit 611be7c into laurent22:master Dec 13, 2019
@archtur
Copy link

archtur commented Dec 17, 2019

Hi devonzuegel, hi laurent,

thank you for your great work here. This feature will help me to correct some theming issues locally, for example inside the new nord theme.

The only issue I have is that I cannot see which css parts are available, because in the mentioned pre built app the dev tools for the "rest of the app" are not available.

My questions are

  1. Why you hide this dev tools (the official dev tools are only shown for the rendered markdown part)?
  2. Couldn't you show the dev tools for the rest of the app as they are enabled inside the tools menu?

This would help everybody to get the most out of this custom css feature implemented by devonzuegel.

Thank you.

@devonzuegel
Copy link
Contributor Author

Hi @NemesiS5. Thanks for the nice words! I'm excited about this feature too.

If you follow the directions in the Debugging the Desktop Application docs, you will be able to see the dev tools. @laurent22 what do you think of making this more directly accessible via a menu?

@laurent22
Copy link
Owner

@devonzuegel, in the next version (already released as a pre-release), the notes will be rendered in an iframe so there will be just a single dev tool for the app and note viewer. The change is already made and the tool will be under the Help menu.

@devonzuegel
Copy link
Contributor Author

Oh lovely, that's an improvement! Thanks for letting me know.

@devonzuegel
Copy link
Contributor Author

I've created a little repo of my custom styles in case they're helpful to anyone else:
https://discourse.joplinapp.org/t/collection-of-custom-css/5428

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants