Limit max-height of CodeMirror editors for issue comment and wiki #18271

sexybiggetje · 2022-01-14T15:50:47Z

On Codeberg.org it was requested to make the .editor-toolbar sticky when editing large wiki posts. This PR makes that happen.
When searching for appropiate issues on the issue tracker, I also came across issue #10675, which requests to make the issue title bar sticky, but the proposed change there included inline changes to the HTML template. Since this issue can be fixed in the same way using CSS-only, I've included it in this PR.

I've included screenshots of both commit's with this PR;

Position sticky added to the wiki editor, screenshot when scrolling.

Positing sticky added to the wiki editor toolbar, screenshot when on top.

Position sticky added to the issue title container, when scrolling.

Position sticky added to the issue title container, when on top.

On codeberg community it was requested to make the wiki editor toolbar sticky for longer wiki posts, so one wouldn't have to scroll to the top to use it. (Reference; https://codeberg.org/Codeberg/Community/issues/533). In order to make this happen, the .editor-toolbar class needs to become position: sticky, and we need to fix it's transparent background and border-bottom. Because the bottom disappears, we add it. This makes the border become a double border, because the CodeMirror area defines borders for all. As such I've added a border-top: none, on the wiki write tab for the CodeMirror class.

In issue go-gitea#10675 it's requested to make the issue bar sticky upon scrolling in the issue view. The proposed change changes inline html, which is not desirable. As such I've added the position sticky option to it's container, and fix the background upon scrolling.

Fix 0px -> 0 to make the linter happy.

web_src/less/_editor.less

@silverwind

As per review of @silverwind change the z-index to it's lowest requirement of 1.

@silverwind

As per review of @silverwind change the z-index to it's lowest requirement of 1.

silverwind · 2022-01-17T13:25:41Z

CI failure is related to #18277. Need to rebase this branch against master.

silverwind · 2022-01-17T23:57:47Z

I'm not sure about the issue title thing. I dislike it very much because it takes away a lot of vertical space for not much gain. I even run userstyle to remove this stickying on GitHub because I hate it so much.

Regarding editor toolbar, I think the issue may be solveable by adding a max-height to the element, that way we don't need the sticky there as well. This is how the code editor already works (max-height: 90vh).

lafriks · 2022-01-18T19:46:47Z

I agree that title is taking too much space to be sticky, if it would autocompact somehow when scrolled than it could be useful but otherwise I'm also dislike it.

sexybiggetje · 2022-01-19T12:58:54Z

Sorry for the delay; I've been offline because of an eye infection.

I don't like the 90vh option. Currently the editor grows in size, which causes the page sidebar to be usable. If you switch to the editor being max-height 90vh, that makes the editor itself become scrollable and you end up with 2 scrollbars.
That's perfectly find UI wise, but I prefer the full page to be scrollable.
Making the title area condensed upon scrolling probably needs an implementation in javascript, I can't think of a way to make that css-only. I don't feel the same way. Maybe a vertical size breakpoint could make it not sticky on smaller displays? Would that be an option?

I'm not sure on both what then the appropiate approach is, one could also make it a user preference. But for things like this I don't like preferences. It should be sane by default and not flood the user with settings.

silverwind · 2022-01-19T17:46:48Z

I think a max-height solution is the way to go. GitHub does the same, you can not extend the editor size greater than the viewport and that way, the controls always stay on screen.

schorsch13 · 2022-02-03T08:16:32Z

@sexybiggetje are you still working on this PR?

schorsch13 · 2022-02-03T08:19:04Z

To sum it up:

Implement the "sticky" toolbar with the max-height option
Ditch the sticky issue header

sexybiggetje · 2022-02-03T08:31:53Z

I should update it, but health came in between. Need a small bit of time

schorsch13 · 2022-02-03T08:34:23Z

I could do it for you if you dont mind

schorsch13 · 2022-02-03T09:33:35Z

I'm not sure about the issue title thing. I dislike it very much because it takes away a lot of vertical space for not much gain. I even run userstyle to remove this stickying on GitHub because I hate it so much.

Regarding editor toolbar, I think the issue may be solveable by adding a max-height to the element, that way we don't need the sticky there as well. This is how the code editor already works (max-height: 90vh).

@silverwind 90vh is too big and it is not scroll-able. The text just disappears

_repository.less

.repository.wiki .tab[data-tab="write"] .CodeMirror {
  max-height: 90vh;
  overflow: scroll;
}

andy5995 · 2022-02-04T05:54:44Z

I could do it for you if you dont mind

@schorsch13 I see that @sexybiggetje gave your comment a thumbs-up, so I assume that means you are clear to finish it up so he can rest easy. ;)

schorsch13 · 2022-02-04T06:31:35Z

@andy5995 thanks for letting me know but I am kinda stuck currently. I added the following part to web_src/less/_editor.less:

.repository.wiki .tab[data-tab="write"] .CodeMirror {
  max-height: 90vh;
  overflow: scroll;
}

The text area does not expand and there is a scrollbar but every time I try to scroll it glitches back to the top.

schorsch13 · 2022-02-04T06:48:03Z

There are also 2 scrollbars

schorsch13 · 2022-02-04T11:20:39Z

@sexybiggetje I think I am not able to implement the scroll-able text area

sexybiggetje · 2022-02-04T15:57:07Z

No worries, I think I'm able to take a look later today. I''sm assuming the second scrollbar is because a calculated problem caused by paddings.

Thanks @andy5995 for responding on my behalf 👌

Fixes the max-height to 85vh, on the proposed 90vh it just came out just slightly too large. Unstickies the changes from the sticky commits.

Removes the changes as done by the sticky title editor.

Remove the max-height in CodeMirror-scroll definition, in order to generalize it in the CodeMirror less file. As per discussion in go-gitea#18271.

sexybiggetje · 2022-02-14T09:02:55Z

I've made the requested change, and moved it into the CodeMirror specific file.

wxiaoguang · 2022-03-22T05:19:37Z

The usage of the CSS styles should be limited to the wiki(or issue) editor elements.

For example, there are other CSS styles for .CodeMirror-scroll:

gitea/web_src/less/_review.less

Lines 202 to 205 in 77d1c7b

    
           #review-box .CodeMirror-scroll { 
        
             min-height: 80px; 
        
             max-height: calc(100vh - 360px); 
        
           }

If we do not isolate these styles, they would conflict in one day.

wxiaoguang · 2022-05-24T07:28:49Z

web_src/less/_base.less

-.dropdown:not(.selection) > .menu.review-box > * {
-  @media (max-height: 700px) {
-    .CodeMirror,
-    .CodeMirror-scroll {
-      min-height: 100px;
-    }
-  }
-}
-


These code has been covered by #19003

#19003

wxiaoguang · 2022-05-24T07:52:50Z

Keep this open for a few days before merge - if people have more thoughts.

About: generalized rules:

If there is a customized class like .full-height-editor .CodeMirror-scroll { max-height: 85vh; }, then it can be used as generalization.
Otherwise, the new customized style max-height: 85vh; shouldn't be applied to the global existing .CodeMirror-scroll, it would lead to conflicts and pollution. There are already a lot of polluted styles in Gitea project, such cases should be avoided as much as possible.

silverwind · 2022-05-24T21:04:11Z

PR title is inaccurate, it does not do any more stickying, just limit the CodeMirror height. I think a similar fix could be done to the <texarea> variant as well. I generally have no issues using a single global for it, ideally with !important to win over more specific rules.

wxiaoguang · 2022-05-25T03:53:31Z

PR title is inaccurate, it does not do any more stickying, just limit the CodeMirror height. I think a similar fix could be done to the <texarea> variant as well. I generally have no issues using a single global for it, ideally with !important to win over more specific rules.

Please do not pollute the global styles, another example, there was #10897 , it says:

/* limit width of all direct dropdown menu children */
/* https://github.com/go-gitea/gitea/pull/10835 */
.dropdown > .menu > * {

Then, how it is now? It becomes very messy code now:

.dropdown:not(.selection) > .menu:not(.review-box) > *:not(.header) { 
...
}

Do you think the code is more and more maintainable?

Even so, many users complains that "why new Gitea make the dropdown so narrow". That's all caused by the pollution from the beginning (#10897). If there was no pollution from the beginning, the fix could be very clear and no more messy work from then on.

silverwind · 2022-05-25T12:46:36Z

Do you think the code is more and more maintainable?

No, but that is literally the worst-case example. Limiting codemirror/textarea height is not something that will require such exception rules, the class is unique enough.

wxiaoguang · 2022-05-25T13:03:46Z

Do you think the code is more and more maintainable?

No, but that is literally the worst-case example. Limiting codemirror/textarea height is not something that will require such exception rules, the class is unique enough.

Every pollution might lead to the worst-case. There are too many examples. Let me find them:

The dialog polluted by text-align: center: #15532 (comment) , the polluted code
left and right are polluted: #19015
blue was polluted by green, fixed by #19763
and more, if you need to know, I can find more and more bad examples.

Sometimes people just imagine that "OK, it's unique/general, let's do the tricky/make the style as the default", but the fact is not what they thought in most cases.

There are so many worst-case examples. Every time a CSS class is polluted, the more messy Gitea's code base becomes.

lunny · 2022-06-04T11:36:17Z

@silverwind @wxiaoguang Is this ready to merge?

* giteaofficial/main: Add alt text to logo (go-gitea#19892) Limit max-height of CodeMirror editors for issue comment and wiki (go-gitea#18271) Implement http signatures support for the API (go-gitea#17565) Increment tests time out from 40m to 50m because sometimes the machine is slow (go-gitea#19887) fix(CI/CD): correct CI variable. (go-gitea#19886) Fix typo (go-gitea#19889) Fixing wrong paging when filtering on the issue dashboard (go-gitea#19801) Move `/info` outside authorization (go-gitea#19888) Fix order by parameter (go-gitea#19849) Exclude Archived repos from Dashboard Milestones (go-gitea#19882) use exact search instead of fuzzy search for branch filter dropdown (go-gitea#19885)

sexybiggetje added 4 commits Jan 14, 2022

Make linter happy on _repository.less

7a4079f

Fix 0px -> 0 to make the linter happy.

Make linter happy on _editor.less

52c9f65

Fix 0px -> 0 to make the linter happy.

axifive added backport/v1.7 kind/ui and removed backport/v1.7 labels Jan 14, 2022

silverwind reviewed Jan 15, 2022

View changes

web_src/less/_editor.less Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 label Jan 15, 2022

sexybiggetje added 2 commits Jan 15, 2022

Change z-index to the lowest boundary of 1

01a1dd4

As per review of @silverwind change the z-index to it's lowest requirement of 1.

Change z-index to the lowest boundary of 1

4c949f9

As per review of @silverwind change the z-index to it's lowest requirement of 1.

Merge branch 'main' into sticky_fixes

59019e4

sexybiggetje added 3 commits Feb 4, 2022

Merge branch 'main' into sticky_fixes

750c20e

Revert changes made to wiki editor (unsticky) and add max-height

52632d9

Fixes the max-height to 85vh, on the proposed 90vh it just came out just slightly too large. Unstickies the changes from the sticky commits.

Revert changes for the sticky title editor

fd31c52

Removes the changes as done by the sticky title editor.

Remove CodeMirror-scroll definition

9246d84

Remove the max-height in CodeMirror-scroll definition, in order to generalize it in the CodeMirror less file. As per discussion in go-gitea#18271.

silverwind approved these changes Feb 14, 2022

View changes

GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 14, 2022

Gusted added this to the 1.17.0 milestone Apr 11, 2022

wxiaoguang added 2 commits May 24, 2022

Merge branch 'main' into sticky_fixes

c0452f9

fine tune CodeMirror min-height/max-height

0fb6b70

wxiaoguang reviewed May 24, 2022

View changes

wxiaoguang approved these changes May 24, 2022

View changes

GiteaBot added lgtm/done and removed lgtm/need 1 labels May 24, 2022

wxiaoguang changed the title ~~Sticky editor toolbar in wiki view and issue view~~ Limit max-height of editors for issue comment and wiki May 25, 2022

wxiaoguang changed the title ~~Limit max-height of editors for issue comment and wiki~~ Limit max-height of CodeMirror editors for issue comment and wiki May 25, 2022

wxiaoguang added 2 commits May 25, 2022

Merge branch 'main' into sticky_fixes

3379059

Merge branch 'main' into sticky_fixes

9f484fb

Merge branch 'main' into sticky_fixes

7030789

lunny added 4 commits Jun 4, 2022

Merge branch 'main' into sticky_fixes

74dee17

Merge branch 'main' into sticky_fixes

c913515

Merge branch 'main' into sticky_fixes

893561b

Merge branch 'main' into sticky_fixes

ea2048d

zeripath merged commit 89a8b3e into go-gitea:main Jun 5, 2022
2 checks passed

May	JUN	Aug
	20
2021	2022	2023

Limit max-height of CodeMirror editors for issue comment and wiki #18271

Limit max-height of CodeMirror editors for issue comment and wiki #18271

sexybiggetje commented Jan 14, 2022

silverwind commented Jan 17, 2022

silverwind commented Jan 17, 2022

lafriks commented Jan 18, 2022

sexybiggetje commented Jan 19, 2022

silverwind commented Jan 19, 2022

schorsch13 commented Feb 3, 2022

schorsch13 commented Feb 3, 2022

sexybiggetje commented Feb 3, 2022

schorsch13 commented Feb 3, 2022

schorsch13 commented Feb 3, 2022 •

edited

andy5995 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

sexybiggetje commented Feb 4, 2022

sexybiggetje commented Feb 14, 2022

wxiaoguang commented Mar 22, 2022

wxiaoguang May 24, 2022

wxiaoguang commented May 24, 2022 •

edited

silverwind commented May 24, 2022

wxiaoguang commented May 25, 2022 •

edited

silverwind commented May 25, 2022

wxiaoguang commented May 25, 2022

lunny commented Jun 4, 2022

Limit max-height of CodeMirror editors for issue comment and wiki #18271

Limit max-height of CodeMirror editors for issue comment and wiki #18271

Conversation

sexybiggetje commented Jan 14, 2022

silverwind commented Jan 17, 2022

silverwind commented Jan 17, 2022

lafriks commented Jan 18, 2022

sexybiggetje commented Jan 19, 2022

silverwind commented Jan 19, 2022

schorsch13 commented Feb 3, 2022

schorsch13 commented Feb 3, 2022

sexybiggetje commented Feb 3, 2022

schorsch13 commented Feb 3, 2022

schorsch13 commented Feb 3, 2022 • edited

andy5995 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

schorsch13 commented Feb 4, 2022

sexybiggetje commented Feb 4, 2022

sexybiggetje commented Feb 14, 2022

wxiaoguang commented Mar 22, 2022

wxiaoguang May 24, 2022

Choose a reason for hiding this comment

wxiaoguang commented May 24, 2022 • edited

silverwind commented May 24, 2022

wxiaoguang commented May 25, 2022 • edited

silverwind commented May 25, 2022

wxiaoguang commented May 25, 2022

lunny commented Jun 4, 2022

schorsch13 commented Feb 3, 2022 •

edited

wxiaoguang commented May 24, 2022 •

edited

wxiaoguang commented May 25, 2022 •

edited