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

Allow custom css UI file #2902

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JustinGeorgi
Copy link
Contributor

This PR checks for a theme.css file served from, OH_CONF/html. If that file exits it links it to the MainUI page after f7 css calculations have been made so that user directives are not over-riden by any f7 settings.

The ability to set MainUI site-wide css has come up several times in forum questions and requests. I think I remember at some point @ghys had some hesitancy with regards to enabling a site-wide css file like this, but I don't remember what the objection was and I can't find where I thought this comes from anymore so I may be misremembering. Either way, this small change just enables an advanced user to create a single css file.

However, if this starts to get a lot of use, I can see refactoring many of the current styles to use a series of --oh- specific css variables which would allow for easy creation and sharing of different MainUI themes (something that has just come up again in the most recent wishlist thread on the forums).

Signed-off-by: Justin Georgi <[email protected]>
Copy link

relativeci bot commented Dec 7, 2024

#2525 Bundle Size — 10.87MiB (~+0.01%).

8cc865d(current) vs 1258cf0 main#2520(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#2525
     Baseline
#2520
Regression  Initial JS 1.91MiB(~+0.01%) 1.91MiB
No change  Initial CSS 577.48KiB 577.48KiB
Change  Cache Invalidation 22.8% 19.4%
No change  Chunks 226 226
No change  Assets 249 249
No change  Modules 2942 2942
No change  Duplicate Modules 152 152
No change  Duplicate Code 1.8% 1.8%
No change  Packages 96 96
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#2525
     Baseline
#2520
Regression  JS 9.08MiB (~+0.01%) 9.08MiB
No change  CSS 864.45KiB 864.45KiB
No change  Fonts 526.1KiB 526.1KiB
No change  Media 295.6KiB 295.6KiB
No change  IMG 140.74KiB 140.74KiB
No change  HTML 1.38KiB 1.38KiB
No change  Other 871B 871B

Bundle analysis reportBranch JustinGeorgi:jag-custom-cssProject dashboard


Generated by RelativeCIDocumentationReport issue

@ghys
Copy link
Member

ghys commented Dec 7, 2024

@JustinGeorgi I don't remember where I made my reservations either but in general I'm very conservative when it comes to letting a simple remote HTTP request (either through the UI or directly) make catastrophic changes (direct JS execution or CSS overrides) that would render the UI inoperable or worse, doing malicious stuff.

I'm also not a fan of just making a request and checking whether there's a 404 response or not. I think it's better to check whether the file exists server-side and add it to a index.html template (which would have to be rendered dynamically).

@JustinGeorgi
Copy link
Contributor Author

I'm also not a fan of just making a request and checking whether there's a 404 response or not. I think it's better to check whether the file exists server-side and add it to a index.html template (which would have to be rendered dynamically).

I'm in complete agreement here, unfortunately what I understand to be the necessary changes to core to implement this method correctly are beyond my current capabilities and also beyond my current time to learn.

I don't even completely disagree with your conservative viewpoint here, but I do think css is a very limited (and 100% easily reversible) risk. So, in my opinion, the pros outweigh the cons here (especially as there are more potentially dangerous alternatives to achieve this if a user is determined but inexperienced).

All that said, feel free to close this if you really think it's a non-starter. This was just a quick proof of concept while I was working on something else, so there's little lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants