mugiwara85 / CodeblockCustomizer

Codeblock Customizer plugin for Obsidian
MIT License
148 stars 7 forks source link

Style elements via css #24

Closed mayurankv closed 10 months ago

mayurankv commented 1 year ago

Sorry for the many issues! It would be good to avoid hardcoding element styles and instead apply the styles to the classes of the elements.

mugiwara85 commented 1 year ago

? What is hardcoded? Styles are applied via css

mayurankv commented 1 year ago

As I understand it, currently css variable like --header-color and so on are directly added as html rather than via css for that element. (Apologies if my plugin is outdated and this has been changed).

For example, doing. Quick inspect element gives me:

<div class="codeblock-customizer-header-language-tag" style="--codeblock-lang-background-color:#B8B5AA; --codeblock-lang-color:#C25F30; --codeblock-lang-bold:bold; --codeblock-lang-italic:italic;">JavaScript</div>

And this style parameter could also be applied by the css

.codeblock-customizer-header-language-tag {
    --codeblock-lang-background-color:#B8B5AA;
    --codeblock-lang-color:#C25F30;
    --codeblock-lang-bold:bold;
    --codeblock-lang-italic:italic;
}

It would be fine to leave it as is and I may be overlooking a reason for adding this directly via html but for myself it means I have to add !important to my css styles to override these settings. (I know that these can be themed via the plugin but I am using css variables consistent with the rest of my theme so changes only need to be made in one place).

mugiwara85 commented 1 year ago

That is the point of using CSS variables. How else would you want to change the colors dynamically without using variables? This is why the settings tab is there. If you change a color, you change the value of a variable, and it gets updated. If I would do as you propose, that would mean, everyone had to use CSS, and every time the color should be changed, then the CSS had to be modified. This goes against the whole point of the plugin. Sorry, but I can't really follow your reasoning/logic here.

mayurankv commented 1 year ago

The css I suggested is still using the same css variables - this could be changed via the settings as well. In fact it would reduce the issue of forcing re-render in reading mode to see changed as the html elements wouldn't need to be changed if a colour changes. Because css changes can be applied without re-rendering the page whereas changing the variable via the element style would require a re-render.

To clarify, this approach still DOES use css variables which can be changed via the plugin settings. No extra css would be needed. It would literally be adding a style.css file containing these definitions which the plugin changes when the user changes a colour.

Note that this style.css would be part of the plugin, not a user stylesheet. To modify the values I would have to write in my own stylesheet to override these settings still but I could do so with a more precise selector as opposed to using !important.

mugiwara85 commented 1 year ago

I have to admit, my CSS knowledge is definitely not the best. I'm open for improvements (definitely regarding CSS), but you have to explain it little more detailed :)

mayurankv commented 1 year ago

No worries! Its definitely not a major issue at all, I just thought the plugin would be a little bit better structured by removing the hardcoding. When I have some time I'll take a look at some other plugins that use css and see how they affect stylesheets and get back to you.

mayurankv commented 1 year ago

I would suggest leaving this issue open as a 'future enhancement' or 'refactor' issue.

mugiwara85 commented 1 year ago

So be it then. I think I will definitely need some help at some point regarding CSS. It should be better structured. Probably that's why there are many problems regarding rendering, because I set the wrong styles :(

mayurankv commented 1 year ago

I'd be happy to assist. I just need to familiarise myself with how obsidian plugins work with css. I think it could be causing conflicts with themes because themes can't necessarily overwrite things. But the rendering is actually to do with the plugin not triggering a refresh of the note on change. Obsidian only re-renders a previewed note on open or some event is triggered, thus when the html changes the changes are not reflected. I would actually suggest a better way of doing this plugin would be via css. So always loading the line numbers and headers and heading-texts etc. Then setting the css display: none to the elements that the user doesn't want to see. Having said this I'm not familiar with javascript nor plugin development so I don't know if this is best practice. May be worth asking on the Obsidian discord?

mayurankv commented 1 year ago

FYI, the method I suggested with using css to hide and show based on settings would not require re-rendering of the page to see changes.

mayurankv commented 1 year ago

(HTML changes are what is causing issues with re-rendering, CSS changes are applied directly)

mayurankv commented 1 year ago

Another refactor to mention would be removing the RV from some classes (I presume reading view?) as this can be selected for with .markdown-rendered for reading view and .markdown-source-view for source mode and live preview.

mugiwara85 commented 1 year ago

Yes, that's true, but as I said, since my CSS knowledge is poor, that would require to redo the Reading View part. I did the best I could :D

mayurankv commented 1 year ago

Leaving the major Reading View refactor for now, I am currently having a look through attempting to make some changes. I wanted to discuss how the styling should be implemented. I believe:

  1. css variables like --codeblock-customizer-line-highlighted-color should be added to body
  2. the uses of these variables (i.e. color: --variable) should be done via styles.css
  3. the elements themselves should only have classes for styling - no direct style attributes.

I've forked the repo and will try and submit a PR with these changes.

Does this seem sensible to you?

mayurankv commented 1 year ago

Actually the custom highlighters will need styles applied directly to them (as will any other procedurally generated items) but that would be fine imo. Setting --codeblock-customizer-line-highlighted-color to whatever the custom highlight colour is in those elements would do the trick.

mugiwara85 commented 1 year ago

What do you mean exactly by the first point? The 2nd is fine, that's how it is now (as of my understanding). 3rd: there must be styling applied.

mayurankv commented 1 year ago

What I mean in point 1 is the variables should be added via document.body.style.setProperty() unless their scope is not global (and with this plugin, the only non-global variables is the different highlight colours).

In point 3, styling will be applied but via the plugin's css stylesheet acting on the classes added by the plugin javascript, not as style attributes added by the plugin javascript. (Note the highlighting custom classes will need some styling via javascript). I'm just about to submit a PR so you can see what I mean.