mugiwara85 / CodeblockCustomizer

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

Feature/css transition CSS Transition V2 #31

Closed mayurankv closed 1 year ago

mayurankv commented 1 year ago

Things that need fixing:

Going to submit incremental commits for these so you can test each fix even if I don't get all done in one go.

mayurankv commented 1 year ago

Can you check the scrolling issue still happens?

mayurankv commented 1 year ago

There is a weird folding issue for a codeblock with fold in the title. On first render, in live preview, the html hyper-MD-codeblock classes aren't added, do you have any idea why this would be?

mayurankv commented 1 year ago

(It only happens on my branch, not the main)

mayurankv commented 1 year ago

Gonna do the theme fix tmrw if possible. Got an idea of where it's going wrong. The way it should work is to add the current theme colors to the main body.codeblock-customizer css identifier with !important to override .theme-light and .theme-dark. Currently, the current theme colors aren't updating properly so I need to fix that logic.

From you I just need confirmation that the previous fixes do indeed work, replication of the folded code live preview render problem and follow up about the scrolling line numbers.

If the line numbers aren't fixed, we're going to have to look at an alternative temporary solution for massive line numbers.

mugiwara85 commented 1 year ago

Gonna do the theme fix tmrw if possible. Got an idea of where it's going wrong. The way it should work is to add the current theme colors to the main body.codeblock-customizer css identifier with !important to override .theme-light and .theme-dark. Currently, the current theme colors aren't updating properly so I need to fix that logic.

From you I just need confirmation that the previous fixes do indeed work, replication of the folded code live preview render problem and follow up about the scrolling line numbers.

If the line numbers aren't fixed, we're going to have to look at an alternative temporary solution for massive line numbers.

Empty heading containers - this is ok.

Border radius applications -this is almost ok. One case I found when it doesn't work correctly is when you define a language for the codeblock which doesn't have an icon. I defined test as a language here: image Can you see the left upper corner? It is not displayed correctly if I enable Always display codeblock language option. If I enable the Always display codeblock language icon option, then it is ok.

Padding in header - this one is not ok. Sometimes it is displayed correctly, but sometimes it is not. image In this example there is also padding and margin applied here which results in displaying double the space that it should. There is also some problem with the height. Create a simple codeblock: ```csharp test ``` Turn on both Always display codeblock language and Always display codeblock language icon options and then turn off and on repeateadly the Always display codeblock language option. You will see that the height of that codeblock is extending and shrinking a few pixels every time.

There is no change regarding the line numbers in editing mode. The scrollbar is still displayed because overflow-x: scroll; is still applied. If you can't fix the line numbers then leave them as they are now, and I will take a look at it when you are done.

There is a weird folding issue for a codeblock with fold in the title.

No, but leave it as well, and I will look at it if you are done. Don't waste any time on it for now.

mayurankv commented 1 year ago

Oh, what system are you on? Could it be that scrollbars don't show up for me until I actually scroll as I'm on macos?

mayurankv commented 1 year ago

Fixed the border radius and header padding edge cases. Can not reproduce the height changes - can you send a video of the issue please (and check it hasn't somehow been fixed by the other changes)?

mayurankv commented 1 year ago

For the line numbers, ok, I've got two last things I want to try. Can you see if it is any better now? If not, uncomment the display: none after .markdown-source-view .codeblock-customizer-line-number::-webkit-scrollbar in styles.css and that should definitely make it work fine.

If it works, I think this should be fine as a temporary fix until the code can be refactored!

mayurankv commented 1 year ago

No, but leave it as well, and I will look at it if you are done. Don't waste any time on it for now.

Ok. It appears in live preview when unfolding a codeblock with both language tags and icons turned off.

mayurankv commented 1 year ago

Could you explain to me exactly how the toggles for new themes work? Is it if you want a theme to be default for light or dark a toggle is set? What would be the difference between setting it and not setting it? Is it just to allow theme switching between changes from light to dark?

Since we plan to refactor the settings anyway and adjusting for this functionality as it is right now would be a bit of a mess, can we just remove this feature for now?

mugiwara85 commented 1 year ago

Could you explain to me exactly how the toggles for new themes work? Is it if you want a theme to be default for light or dark a toggle is set? What would be the difference between setting it and not setting it? Is it just to allow theme switching between changes from light to dark?

Since we plan to refactor the settings anyway and adjusting for this functionality as it is right now would be a bit of a mess, can we just remove this feature for now?

If you created a theme, which you want to use as a default light theme, then you set the toggle when saving the theme. The same for dark. This results in that when you change obsidian's theme from light to dark, then automatically the dark theme you saved as default will be applied. Same for light as well. You are not required to use the toggles. You can just save a theme without a toggle. In this case it will not follow obsidian's theme change and is always the same theme applied. So basically, if you want the theme to change automatically when you change obsidian's theme, then you can do it. If you want to always use the same theme regardless of what obsidian's theme is, then you can do that as well.

I would not remove that feature. That would cause a lot of issues opened, since people never read anything :D

mayurankv commented 1 year ago

Oh, I've misunderstood the feature! It works fine as is.

mayurankv commented 1 year ago

Believe I've fixed the theme issue as well. Please let me know if this works for you.

mayurankv commented 1 year ago

One more bug i've found (also in your master branch) is that an empty codeblock with an empty line between the delimiters shows no line numbers in reading mode, but with two empty lines, it shows the line numbers properly.

mugiwara85 commented 1 year ago

Border radius and padding are both ok now, but the height change not. Here is a video:

https://github.com/mugiwara85/CodeblockCustomizer/assets/65603765/ab7f14e1-964a-4db1-9d62-3ad967264697

mugiwara85 commented 1 year ago

One more bug i've found (also in your master branch) is that an empty codeblock with an empty line between the delimiters shows no line numbers in reading mode, but with two empty lines, it shows the line numbers properly.

Yeah, I knew about that, but never understood why it won't work :D

mugiwara85 commented 1 year ago

This is how the line number look now in editing view: image After uncommenting display: none it is a bit better, but not quite: image

mayurankv commented 1 year ago

I'll look into it whilst you check the other fixes. Have you tried the line number fix?

mayurankv commented 1 year ago

That is what I wanted them to look like!

mayurankv commented 1 year ago

It's just a temporary fix and far from the best one but it's going to need some javascript sizing to make gutter expanding to fit the numbers work properly - css won't cut it. I think it would be best fixed properly after the refactor.

Also note that those lines can be scrolled to see the full number. This way you get the gist of line numbering without losing anything for now.

mugiwara85 commented 1 year ago

Themes: There are a few errors here:

mayurankv commented 1 year ago

Ok, I'll try and fix those two problems as well as the changing height header.

mugiwara85 commented 1 year ago

Ok, I'll try and fix those two problems as well as the changing height header.

Could you reproduce that (the height problem)? It is definitely something small

mayurankv commented 1 year ago

Yes I could and I think I've fixed it now. It was to do with the language tag filling as much space as it could which was dependent on font size. I could have made the font smaller but I just forced a height equal to that of the icon to the header container so a larger font-size will just push the text out of sight (though this isn't a user setting so there's no danger of that).

mayurankv commented 1 year ago

Sorry, the height fix had a small issue with conflicting with an existing css variable so that's been fixed. I also think the updating colors instantly as well as themes are fixed but let me know on that front. Looking into the setting default themes now.

mugiwara85 commented 1 year ago

The height issue is ok now! Colors change immediately now, this is also ok! Then only the themes problem remains, right?

mayurankv commented 1 year ago

I believe themes work. Just not the default light and dark. Just doing something else and I'll try sort it out by the end of the day.

mayurankv commented 1 year ago

Ok now default theming is fixed as well.

mayurankv commented 1 year ago

Should all be finally done haha.

mayurankv commented 1 year ago

Eurgh, there's still an issue with the header sub-element padding. Fixing it now.

mayurankv commented 1 year ago

Done now. Let me know when you manage to check them. If you then merge your changes and publish the new version, I'll start working on a refactor.

mayurankv commented 1 year ago

I added a fix for empty preview codeblocks showing line numbers. The side effect is that line numbers are now shown even for a completely empty codeblock as in:

I think this is completely reasonable though and it makes a lot more sense for one empty line and no empty lines to show a line number of 1 than no line number given 2,3,... empty lines do show line numbers.

mugiwara85 commented 1 year ago

I added a fix for empty preview codeblocks showing line numbers. The side effect is that line numbers are now shown even for a completely empty codeblock as in:

Oh, but that's not ok. The reason I did not anything about it, because it is a very special case. I don't think anyone will have an empty code block. If you enter text in the line then the line number is shown.

mayurankv commented 1 year ago

What's wrong with the fix though? The only unintended consequence is completely empty codeblocks show a line number of 1. Is this problematic? Why?

mugiwara85 commented 1 year ago

What's wrong with the fix though? The only unintended consequence is completely empty codeblocks show a line number of 1. Is this problematic? Why?

Well, there is no line in the code block :)

mugiwara85 commented 1 year ago

What's wrong with the fix though? The only unintended consequence is completely empty codeblocks show a line number of 1. Is this problematic? Why?

I just checked. Leave it this way. Obsidian renders such code blocks with one line. Still incorrect, but if Obsidian does this, then you can leave it as well.

mayurankv commented 1 year ago

Have you had a chance to check the final corrections?

mugiwara85 commented 1 year ago

I checked everything. Everything works perfectly. I found only these 2 problems:

mayurankv commented 1 year ago

Note this PR addresses issues #24, #27 and #16.

Can I confirm that you have additional fixes that would address issues #19, #21, #22 and #17.

Furthermore issue #29 could do with renaming and keeping as a meta issue.

mugiwara85 commented 1 year ago

Note this PR addresses issues #24, #27 and #16.

Can I confirm that you have additional fixes that would address issues #19, #21, #22 and #17.

Furthermore issue #29 could do with renaming and keeping as a meta issue.

29 Could be anything. The user hasn't told any new info, and I can't read minds, so it will have to wait.

For #21 I do not yet have a solution, since I wanted to wait for you first. For #19 #22 and #27 I have my solution ready, Just waiting for this PR.

mayurankv commented 1 year ago

Corrected both of the issues. Let me know if its fine now.

mayurankv commented 1 year ago

29 appears to be regarding the issue of reading mode not re-rendering when changing codeblock parameters after the language. But I'll make a new issue for that and just link it instead.

I'll have a look into #21.

mugiwara85 commented 1 year ago

Please tell me when you are done. Until then it has no point of checking everything and merging the code if you still change it :)

mayurankv commented 1 year ago

Sorry! I am actually done now. I won't make any changes till you've tested. I just found a small bug whilst I was looking into the hover issue.

mugiwara85 commented 1 year ago

No worries! I start checking!

mugiwara85 commented 1 year ago

Oh wait. You broke something. The alternative highlight color doesn't work anymore: image Also the issue reappeared when very long line has no space in it. image What did you change?

mayurankv commented 1 year ago

Woah what? I have neither of those issues?

mayurankv commented 1 year ago
image
mugiwara85 commented 1 year ago

Woah what? I have neither of those issues?

In edit mode?

mugiwara85 commented 1 year ago

Oh wait. Alternate highlighting works now. Interesting.