Frontend refactor, PascalCase to camelCase, remove unused code #17365

wxiaoguang · 2021-10-20T08:20:45Z

wxiaoguang commented

2021-10-20 08:20:45 +00:00

As discussed in Frontend Plan https://github.com/go-gitea/gitea/issues/17149 and https://github.com/go-gitea/gitea/pull/17315 , we'd like to use camelCase instead of PascalCase for keys in window.config

This PR does the rename, and improve some code.

Changes:

All keys in window.config are camelCase now.
PageIsProjects is not necessary, related templates are repo/projects/new.tmpl and repo/projects/view.tmpl, the logic is replaced by checking CSS names $('.repository.projects')
RequiresDraggable in repo/projects.go is never used in whole project, so it is removed. Here was a bug: guest users can also drag the issues between board (the update is denied of course), this PR doesn't touch it, it may be fixed in future.
csrf is renamed to csrfToken to make it clear, it is a token.
HighlightJS is never used in whole project and removed. The usage of template data .RequireHighlightJS is documented so keep it as it is.
SimpleMDE is never used and removed.
NotificationSettings is rendered directly by the map generated by backend
<meta name="_csrf" content="{{.CsrfToken}}" /> in <head> is removed, no code uses it, and we already have csrfToken in window.config.

This PR may break something if some users are using window.config.HighlightJS or window.config.SimpleMDE, so I think we mark this PR as a breaking change. Users who require these two options can check or inject {{.RequireHighlightJS}} or {{.RequireSimpleMDE}} in their own custom templates.

As discussed in Frontend Plan https://github.com/go-gitea/gitea/issues/17149 and https://github.com/go-gitea/gitea/pull/17315 , we'd like to use camelCase instead of PascalCase for keys in `window.config` This PR does the rename, and improve some code. Changes: * All keys in `window.config` are camelCase now. * `PageIsProjects` is not necessary, related templates are `repo/projects/new.tmpl` and `repo/projects/view.tmpl`, the logic is replaced by checking CSS names `$('.repository.projects')` * `RequiresDraggable` in `repo/projects.go` is never used in whole project, so it is removed. Here was a bug: guest users can also drag the issues between board (the update is denied of course), this PR doesn't touch it, it may be fixed in future. * `csrf` is renamed to `csrfToken` to make it clear, it is a token. * `HighlightJS` is never used in whole project and removed. The usage of template data `.RequireHighlightJS` is documented so keep it as it is. * `SimpleMDE` is never used and removed. * `NotificationSettings` is rendered directly by the map generated by backend * `<meta name="_csrf" content="{{.CsrfToken}}" />` in `<head>` is removed, no code uses it, and we already have `csrfToken` in `window.config`. This PR may break something if some users are using `window.config.HighlightJS` or `window.config.SimpleMDE`, so I think we mark this PR as a breaking change. Users who require these two options can check or inject {{.RequireHighlightJS}} or {{.RequireSimpleMDE}} in their own custom templates.

🎉 2

silverwind (Migrated from github.com) reviewed 2021-10-20 10:32:22 +00:00

templates/base/head.tmpl

						
				@ -54,7 +45,7 @@

								{{ end }}

silverwind (Migrated from github.com) commented

2021-10-20 10:32:22 +00:00

Maybe expose runMode directly? Thought it would have to be compared insensitively, e.g. runMode.toLowerCase() === 'prod'.

Maybe expose `runMode` directly? Thought it would have to be compared insensitively, e.g. `runMode.toLowerCase() === 'prod'`.

wxiaoguang reviewed 2021-10-20 10:34:46 +00:00

templates/base/head.tmpl

						
				@ -54,7 +45,7 @@

								{{ end }}

wxiaoguang commented

2021-10-20 10:34:46 +00:00

But it's not guaranteed what value a user may set, the backend code is:

func IsProd() bool {
	return RunMode == "prod"
}

I do not want to touch it or couple with the setting option logic.

If we use runMode.toLowerCase() === 'prod' in front code, then we have two magic strings prod ....

I like magic but I do not like magic values. ?

But it's not guaranteed what value a user may set, the backend code is: ``` func IsProd() bool { return RunMode == "prod" } ``` I do not want to touch it or couple with the setting option logic. If we use `runMode.toLowerCase() === 'prod'` in front code, then we have two magic strings `prod` .... I like magic but I do not like magic values. ?

silverwind (Migrated from github.com) reviewed 2021-10-20 10:35:57 +00:00

templates/base/head.tmpl

						
				@ -54,7 +45,7 @@

								{{ end }}

silverwind (Migrated from github.com) commented

2021-10-20 10:35:57 +00:00

Okay we can keep it as boolean, but I'd suggest shortening to isProd.

Okay we can keep it as boolean, but I'd suggest shortening to `isProd`.

wxiaoguang reviewed 2021-10-20 10:46:35 +00:00

templates/base/head.tmpl

						
				@ -54,7 +45,7 @@

								{{ end }}

wxiaoguang commented

2021-10-20 10:46:34 +00:00

It was isProd but it was too short, I worry about the name conflicts with others, so I changed the name a little longer. For global names, a little longer really helps, we can search the name easily from all files.

runModeIsProd is still shorter than assetUrlPrefix anyway, and we still have a chance to introduce runModeValue in future.

It was `isProd` but it was too short, I worry about the name conflicts with others, so I changed the name a little longer. For global names, a little longer really helps, we can search the name easily from all files. `runModeIsProd` is still shorter than `assetUrlPrefix` anyway, and we still have a chance to introduce `runModeValue` in future.

6543 (Migrated from github.com) approved these changes 2021-10-20 14:50:55 +00:00

delvh reviewed 2021-10-20 20:43:46 +00:00

delvh left a comment

Are you sure that SimpleMDE is no longer needed anymore?
Isn't that a separate pull request that is quite a bit larger (#15394)?

Are you sure that SimpleMDE is no longer needed anymore? Isn't that a separate pull request that is quite a bit larger (#15394)?

web_src/js/publicpath.js

						
				@ -3,3 +3,3 @@

				import {joinPaths} from './utils.js';

				const {AssetUrlPrefix} = window.config;

				const {assetUrlPrefix} = window.config;

delvh commented

2021-10-20 20:39:37 +00:00

```suggestion ```

web_src/js/publicpath.js

delvh commented

2021-10-20 20:39:29 +00:00

__webpack_public_path__ = joinPaths(window.config.assetUrlPrefix, '/');

Since GitHub suddenly decided to break multiline suggestions, here you have it in two separate suggestions.

```suggestion __webpack_public_path__ = joinPaths(window.config.assetUrlPrefix, '/'); ``` Since GitHub suddenly decided to break multiline suggestions, here you have it in two separate suggestions.

wxiaoguang reviewed 2021-10-21 02:50:27 +00:00

web_src/js/publicpath.js

wxiaoguang commented

2021-10-21 02:50:27 +00:00

No, I won't touch existing logic if it is unrelated during the refactoring. I only change the code it has to be changed.

wxiaoguang reviewed 2021-10-21 02:50:30 +00:00

web_src/js/publicpath.js

						
				@ -3,3 +3,3 @@

				import {joinPaths} from './utils.js';

				const {AssetUrlPrefix} = window.config;

				const {assetUrlPrefix} = window.config;