Skip to content

feat(web): custom stylesheets - #4602

Merged
alextran1502 merged 12 commits into
immich-app:mainfrom
Wingysam:main
Oct 23, 2023
Merged

feat(web): custom stylesheets#4602
alextran1502 merged 12 commits into
immich-app:mainfrom
Wingysam:main

Conversation

@Wingysam

@Wingysam Wingysam commented Oct 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This draft pull request is a work-in-progress at implementing a Custom CSS setting to the system settings page.
image

@vercel

vercel Bot commented Oct 23, 2023 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Oct 23, 2023 5:57pm

martabal
martabal reviewed Oct 23, 2023
View reviewed changes
Comment thread web/src/routes/admin/system-settings/+page.svelte Outdated
@martabal

martabal commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Can't wait to see custom themes 😎

@jrasm91 jrasm91 changed the title Custom Stylesheets web: custom stylesheets Oct 23, 2023
@Wingysam

Wingysam commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

All functionality is working as intended and all web/server/e2e tests are passing locally. Ready for review!

@Wingysam
Wingysam marked this pull request as ready for review October 23, 2023 03:26
jrasm91
jrasm91 approved these changes Oct 23, 2023
View reviewed changes

@jrasm91 jrasm91 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm

@etnoy

etnoy commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator

Haven't checked the code itself, but love to see the theming support!

jrasm91
jrasm91 approved these changes Oct 23, 2023
View reviewed changes

@jrasm91 jrasm91 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, thanks for working on it.

Everything is great, except I would have preferred different names for stuff, like:

  • Admin Settings > Theme (or Theme Customizations or Custom Theme)
  • theme.customCss
  • THEME_CUSTOM_CSS
  • theme-settings.svelte
  • etc.

Stylesheet is very specific and exactly what you implemented, but I think a more general "theme" naming would have been more appropriate.

I am OK merging it as is, if you do not want to change it. If more related changes are suggested in the future, I will require it at that time. Let me know if this is something you would like to address now, if not I will merge it as is.

@Wingysam

Wingysam commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

@jrasm91 I've made the requested changes.

@Wingysam

Wingysam commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

Oops, forgot to update the server test and run the tests for the server. Fixed that and tests pass now.

@alextran1502 alextran1502 changed the title web: custom stylesheets feat(web): custom stylesheets Oct 23, 2023
@alextran1502
alextran1502 enabled auto-merge (squash) October 23, 2023 18:26
@alextran1502
alextran1502 merged commit 62a1128 into immich-app:main Oct 23, 2023
@jrasm91

jrasm91 commented Oct 23, 2023

Copy link
Copy Markdown
Member

@jrasm91 I've made the requested changes.

Thank you so much 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrasm91 jrasm91 jrasm91 approved these changes
+1 more reviewer
@martabal martabal martabal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Wingysam @martabal @etnoy @jrasm91 @alextran1502