fix(web): extra spaces in HTML attribute #21855

kecrily · 2022-11-18T07:10:17Z

Remove extra spaces in class attributes caused by conditional statements

Example:

<div class="{{if .Source.IsLDAP}}required{{end}} field">

When IsLDAP is false, will render <div class=" field"> not <div class="field">. So should put space in conditional statements.

Changes are made by VS Code regex replace and are checked manually.

class="\{\{if (.*)\{\{end\}\} -> class="{{if $1 {{end}\}
class="(.*) \{\{if (.*)\}\}(.*)\{\{end\}\} -> class="$1{{if $2}} $3{{end}}

delvh

Your automatic check seems not to have included all cases.

Should we also include the other case char {{<something>}}class in this PR, or do we open another PR for it?

templates/repo/issue/view_content/sidebar.tmpl

templates/repo/issue/list.tmpl

silverwind · 2022-11-18T10:47:26Z

If you are really sure about your regexes, you could put them below this one as a sed replacement for auto-formatting during make fmt:

gitea/Makefile

Line 271 in 0b993a0

    
           @$(SED_INPLACE) -e 's/{{[ 	]\{1,\}/{{/g' -e '/^[ 	]\{1,\}}}/! s/[ 	]\{1,\}}}/}}/g' $(TEMPLATES)

kecrily · 2022-11-18T11:24:25Z

In fact I'm not very confident in my regex. I was afraid it would inadvertently destroy something.

templates/user/dashboard/milestones.tmpl

templates/repo/settings/protected_branch.tmpl

templates/repo/issue/list.tmpl

silverwind · 2022-11-19T18:56:09Z

Need to rebase.

silverwind · 2022-11-20T19:37:48Z

Test failures do seem related, but it's not immediately clear to my why.

zeripath · 2022-11-20T19:41:36Z

Test failures do seem related, but it's not immediately clear to my why.

--- FAIL: TestPullCompare (0.43s)
    testlogger.go:78: 2022/11/20 15:36:35 ...eb/routing/logger.go:99:func1() [I] [637a4983] router: completed GET /user2/repo1/pulls for , 200 OK in 98.0ms @ repo/issue.go:408(repo.Issues)
    pull_compare_test.go:27: 
        	Error Trace:	/drone/src/integration_test.go:337
        	            				/drone/src/integration_test.go:166
        	            				/drone/src/pull_compare_test.go:27
        	Error:      	Not equal: 
        	            	expected: 200
        	            	actual  : 404
        	Test:       	TestPullCompare
        	Messages:   	Request: GET /user2/repo1/compare/master...master%20
    pull_compare_test.go:27: Response: Not found.

In particular:

Messages:   	Request: GET /user2/repo1/compare/master...master%20

I suspect the changes have broken the url somehow.

templates/repo/diff/compare.tmpl

zeripath · 2022-11-20T19:49:46Z

The above two things should fix the problem but we should really go through this PR and double check that it's doing the correct thing.

kecrily · 2022-11-21T05:06:40Z

The issue was constructed with so many files (a mind-boggling 148) that regex were my only option. My regex isn't very good, therefore it could have resulted in an incorrect alteration.

I also made an effort to go back and revert any incorrect alterations, but it was simply too much. Inevitably, something is missed, and we can only rely on the keen eye of the reviewer.

silverwind · 2022-11-21T19:56:50Z

First failure on CI is odd because it's being seen outside this PR too, so I think it may be flaky/unrelated. See #21854.

templates/repo/diff/compare.tmpl

silverwind · 2022-11-25T19:25:02Z

Can you get the tests to pass please? Then I'm happy to review.

templates/repo/issue/list.tmpl

zeripath · 2022-11-25T20:56:15Z

There were two more mistakenly added spaces.

wxiaoguang · 2023-02-14T12:10:04Z

I have objection to this change, and suggest to stick to class="c1 {{if $var}}c2{{end}}"

The readability and maintainability should be applied to the code which is read by developers, but not for the generated outputs.

The template code is the code for developers, while the generated HTML are only for browsers.

The class="c1 {{if $var}}c2{{end}}" style is clearer for developers and more intuitive, and the generated HTML also makes browsers happy (a few spaces do not affect anything)

Think about a more complex case:

class="{{if $active}}active{{end}} menu item {{if $show}}show{{end}} {{if $warn}}warn{{end}}"
--vs--
class="{{if $active}}active {{end}}menu item{{if $show}} show{{end}}{{if $warn}} warn{{end}}"

The first style make it clearer to see each CSS class name with its {{if}} block.

zeripath · 2023-02-14T14:21:02Z

I do agree with @wxiaoguang here - I know we've done a lot of work elsewhere getting rid of the extra spaces but I'm just not sure it's worth it given the confusion and potential for bugs it adds.

Who is looking at the HTML anyway? We'd potentially be better off passing the output of our HTML renderer through a lightweight html parser that would minimize and remove the extra spaces instead.

silverwind · 2023-02-14T19:27:43Z

Not sure if postprocessing the HTML is worth it, it'll impact performance for neglible gain.

BTW, In JS land, there is the classnames module that's used to join together class names, maybe something similar could be done for go templates as well, thought it'll never be as elegant as the JS solution.

### The CustomEvent prefix There was already `ce-quick-submit`, the `ce-` prefix seems better than `us-`. Rename the only `us-` prefixed `us-load-context-popup` to `ce-` prefixed. ### Styles and Attributes in Go HTML Template #21855 (comment) Suggest to stick to `class="c1 {{if $var}}c2{{end}}"` The readability and maintainability should be applied to the code which is read by developers, but not for the generated outputs. The template code is the code for developers, while the generated HTML are only for browsers. The `class="c1 {{if $var}}c2{{end}}"` style is clearer for developers and more intuitive, and the generated HTML also makes browsers happy (a few spaces do not affect anything) Think about a more complex case: * `class="{{if $active}}active{{end}} menu item {{if $show}}show{{end}} {{if $warn}}warn{{end}}"` * --vs-- * `class="{{if $active}}active {{end}}menu item{{if $show}} show{{end}}{{if $warn}} warn{{end}}"` The first style make it clearer to see each CSS class name with its `{{if}}` block. Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

wolfogre · 2023-03-20T01:21:51Z

Now that #23298 has been merged, I suggest closing this PR. @kecrily, thank you for your effort; it prompted us to think more deeply..

fix(web): extra spaces in the class attribute

6ba0de9

delvh reviewed Nov 18, 2022

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 18, 2022

fix: cover more case

96e18ee

kecrily added 6 commits November 18, 2022 19:41

fix: cover more case

9094834

fix: rewrite some condition

ca91345

fix: cover data-need-clear attrbute

de2cd6f

fix: revert some edge cases

8f8cbd2

fix: revert edge cases

d144089

chore: remove duplicate space

f9cdcf3

silverwind reviewed Nov 18, 2022

View reviewed changes

templates/user/dashboard/milestones.tmpl Outdated Show resolved Hide resolved

silverwind reviewed Nov 18, 2022

View reviewed changes

templates/repo/settings/protected_branch.tmpl Outdated Show resolved Hide resolved

silverwind reviewed Nov 18, 2022

View reviewed changes

templates/repo/issue/list.tmpl Outdated Show resolved Hide resolved

fix: use correct if and syntax

96988ea

silverwind mentioned this pull request Nov 19, 2022

fix(web): keep the pages of the navigation in the center #21867

Merged

kecrily added 2 commits November 19, 2022 21:40

fix: cover more cases

8a28d04

fix: cleanup

1ac5ee0

kecrily added 3 commits November 20, 2022 20:33

Merge branch 'main' into fix/extra-spaces

55144de

Merge branch 'main' into fix/extra-spaces

f1d82ee

fix: double check

0e69760

zeripath reviewed Nov 20, 2022

View reviewed changes

templates/repo/diff/compare.tmpl Outdated Show resolved Hide resolved

zeripath reviewed Nov 20, 2022

View reviewed changes

templates/repo/diff/compare.tmpl Outdated Show resolved Hide resolved

fix: data-url change by mistake

08656d0

kecrily changed the title ~~fix(web): extra spaces in the class attribute~~ fix(web): extra spaces in HTML attribute Nov 21, 2022

kecrily added 2 commits November 21, 2022 13:31

fix: cleanup

af95725

Merge branch 'main' into fix/extra-spaces

8f7b345

zeripath reviewed Nov 21, 2022

View reviewed changes

templates/repo/diff/compare.tmpl Outdated Show resolved Hide resolved

zeripath reviewed Nov 21, 2022

View reviewed changes

templates/repo/diff/compare.tmpl Outdated Show resolved Hide resolved

zeripath reviewed Nov 25, 2022

View reviewed changes

templates/repo/issue/list.tmpl Outdated Show resolved Hide resolved

zeripath reviewed Nov 25, 2022

View reviewed changes

templates/repo/issue/list.tmpl Outdated Show resolved Hide resolved

kecrily added 3 commits November 28, 2022 23:01

revert: space by mistake

ed6864d

Merge branch 'main' into fix/extra-spaces

454ec4c

Merge branch 'main' into fix/extra-spaces

d65d319

wxiaoguang mentioned this pull request Mar 4, 2023

Improve the frontend guideline #23298

Merged

wolfogre closed this Mar 20, 2023

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): extra spaces in HTML attribute #21855

fix(web): extra spaces in HTML attribute #21855

kecrily commented Nov 18, 2022 •

edited

Loading

delvh left a comment

silverwind commented Nov 18, 2022 •

edited

Loading

kecrily commented Nov 18, 2022

silverwind commented Nov 19, 2022

silverwind commented Nov 20, 2022 •

edited

Loading

zeripath commented Nov 20, 2022

zeripath commented Nov 20, 2022

kecrily commented Nov 21, 2022

silverwind commented Nov 21, 2022

silverwind commented Nov 25, 2022

zeripath commented Nov 25, 2022

wxiaoguang commented Feb 14, 2023 •

edited

Loading

zeripath commented Feb 14, 2023

silverwind commented Feb 14, 2023

wolfogre commented Mar 20, 2023

fix(web): extra spaces in HTML attribute #21855

fix(web): extra spaces in HTML attribute #21855

Conversation

kecrily commented Nov 18, 2022 • edited Loading

delvh left a comment

Choose a reason for hiding this comment

silverwind commented Nov 18, 2022 • edited Loading

kecrily commented Nov 18, 2022

silverwind commented Nov 19, 2022

silverwind commented Nov 20, 2022 • edited Loading

zeripath commented Nov 20, 2022

zeripath commented Nov 20, 2022

kecrily commented Nov 21, 2022

silverwind commented Nov 21, 2022

silverwind commented Nov 25, 2022

zeripath commented Nov 25, 2022

wxiaoguang commented Feb 14, 2023 • edited Loading

zeripath commented Feb 14, 2023

silverwind commented Feb 14, 2023

wolfogre commented Mar 20, 2023

kecrily commented Nov 18, 2022 •

edited

Loading

silverwind commented Nov 18, 2022 •

edited

Loading

silverwind commented Nov 20, 2022 •

edited

Loading

wxiaoguang commented Feb 14, 2023 •

edited

Loading