Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve theme display #30671

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Apr 24, 2024

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 24, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 24, 2024
@wxiaoguang wxiaoguang marked this pull request as draft April 24, 2024 01:44
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 24, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Apr 24, 2024
@wxiaoguang wxiaoguang changed the title WIP: Improve theme Improve theme display Apr 24, 2024
@wxiaoguang wxiaoguang marked this pull request as ready for review April 24, 2024 06:40
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 25, 2024
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to move variable to :root.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 25, 2024
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 25, 2024

Need to move variable to :root.

I won't do it, unless you could write clear code to implement it, but I think it is impossible (not feasible in my mind)

@silverwind
Copy link
Member

I don't see why a regex like --theme-display-name:\s?(\S+) wouldn't work.

@silverwind
Copy link
Member

silverwind commented Apr 25, 2024

Easy match: https://regex101.com/r/nJEo79/3

@wxiaoguang
Copy link
Contributor Author

I don't see why a regex like --theme-display-name:\s?(\S+) wouldn't work.

Just do it.

@silverwind
Copy link
Member

silverwind commented Apr 25, 2024

Working on new regex.

@silverwind
Copy link
Member

Ugh thanks for the merge conflict.

@wxiaoguang
Copy link
Contributor Author

Ugh thanks for the merge conflict.

It's the same time as answering #30671 (comment) ... the conflict could be simple because I didn't change much, only removed PreferColorSchemes and added some comments

@silverwind
Copy link
Member

silverwind commented Apr 25, 2024

New version is up. It parses for the last occurence of a variable in a file. If that does not suit you, we install a full CSS parser.

@lunny
Copy link
Member

lunny commented May 6, 2024

I think the difference between the two solution is not big. But for @wxiaoguang 's solution, all the theme meta info will be under a private namespace gitea-theme-meta-info which is better than putting them in a public :root namespace to avoid mistake usage.

@jolheiser
Copy link
Member

Why not some form of parse-able frontmatter contained within a defined CSS comment? (Perhaps I missed some prior discussion. Sorry if so.)

/** gitea-meta
display-name: Dark
**/

and parse it as YAML (or JSON, INI, etc.)

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented May 6, 2024

Why not some form of parse-able frontmatter contained within a defined CSS comment? (Perhaps I missed some prior discussion. Sorry if so.)

Because many frontend compilers remove all the comments.

@jolheiser
Copy link
Member

I have no real preference between the two proposed solutions.

I don't love magical CSS being parsed/scraped for metadata. I don't want to "concern stall" this PR into the ground, though, so if no one else has similar concerns I will leave it at that.

/* more custom theme variables ... */
}
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs example how to implement a "auto" theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it needs an example, just copy the "gitea-auto.css"

We can't teach everything here, just like we don't need to explain the --color-xxx one by one.

If you have ideas about how to implement an "auto" theme, feel free to edit the documents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the document fdfd440

@silverwind
Copy link
Member

silverwind commented May 7, 2024

I still want it in :root. One usage example might be if the theme wants to display it's name in the footer. This is impossible with that non-existant selector:

footer .left-links::after {
  content: var(--theme-name);
}

As CSS gets more advanced, more such use cases will arise and we are blocking us out of them by making the variables unavailable to CSS.

I find it insane that we declare variables that are out of variable scope. It's like a unused variable.

@wxiaoguang
Copy link
Contributor Author

I still want it in :root. One usage example might be if the theme wants to display it's name in the footer. This is impossible with that non-existant selector:

footer .left-links::after {
  content: var(--theme-name);
}

As CSS gets more advanced, more such use cases will arise and we are blocking us out of them by making the variables unavailable to CSS.

I find it insane that we declare variables that are out of variable scope. It's like a unused variable.

I have also explained the problem above: it is absolutely an abuse. The "display name" should be prepared by backend code and provided by data-theme-display-name=xxxx, but not directly read by JS/CSS.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels May 24, 2024
@silverwind
Copy link
Member

I guess I won't block any more because the changes are needed for later work, but I won't approve it either until it's changed to use :root.

@silverwind
Copy link
Member

silverwind commented May 24, 2024

Another we should just put put it into a "legal comment" block at the start, arguably better then a CSS block that never matches anything.

/*!
name: My Theme
url: https://gitea.com/my/theme
*/

Or even simpler:

/*! My Theme - https://gitea.com/my/theme */

@jolheiser
Copy link
Member

Another we should just put put it into a "legal comment" block at the start, arguably better then a CSS block that never matches anything.

/*!
name: My Theme
url: https://gitea.com/my/theme
*/

Or even simpler:

/*! My Theme - https://gitea.com/my/theme */

I agree
But #30671 (comment)

@silverwind
Copy link
Member

Why not some form of parse-able frontmatter contained within a defined CSS comment? (Perhaps I missed some prior discussion. Sorry if so.)

Because many frontend compilers remove all the comments.

By putting a ! first character into a comment, it becomes a legal comment which by default many minifiers retain in the output.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@wxiaoguang wxiaoguang removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@wxiaoguang wxiaoguang marked this pull request as draft May 27, 2024 10:25
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented May 27, 2024

Why not some form of parse-able frontmatter contained within a defined CSS comment? (Perhaps I missed some prior discussion. Sorry if so.)

Because many frontend compilers remove all the comments.

By putting a ! first character into a comment, it becomes a legal comment which by default many minifiers retain in the output.

Then it sill needs some "tag" to help the locate the comment block.

Does this look good to you? Or other ideas?

/*!gitea-theme-meta-info
display-name: My Theme
*/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/docs modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants