Skip to content

Use default system primary #34437

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 13, 2022
Merged

Use default system primary #34437

merged 4 commits into from
Oct 13, 2022

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 5, 2022 •
edited
Loading

Fix #34431

  • Fix usage of system primary colour
  • Added color picker
  • Fixed checkbox color on background/colors

image

@skjnldsv skjnldsv added this to the Nextcloud 26 milestone Oct 5, 2022
@skjnldsv skjnldsv requested review from a team, Pytal, jancborchardt and szaimen October 5, 2022 10:15
@skjnldsv skjnldsv self-assigned this Oct 5, 2022
@skjnldsv skjnldsv requested review from PVince81 and nimishavijay and removed request for a team October 5, 2022 10:15
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2022

I think we still need a system config forbidding any user-custom primary/background.
@jancborchardt can we take the "login image" from the admin theming as the default background image if enforced?
image

I can definitely imagine a lot of customers would like to block their users from customizing their cloud instances.
Meaning we should let them the opportunity to specify/change the background and enforce it on all users. This seems like a good source

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 5, 2022
View reviewed changes
@szaimen
Copy link
Contributor

szaimen commented Oct 5, 2022 •
edited by skjnldsv
Loading

I think we still need a system config forbidding any user-custom primary/background. @jancborchardt can we take the "login image" from the admin theming as the default background image if enforced? image

I can definitely imagine a lot of customers would like to block their users from customizing their cloud instances. Meaning we should let them the opportunity to specify/change the background and enforce it on all users. This seems like a good source

Makes sense to me and sounds good!

@skjnldsv skjnldsv force-pushed the feat/theming-default-system-value branch from ce7d8be to a8d6143 Compare October 5, 2022 11:05
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2022

Added tests and use-cases + fixed existing testing

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 5, 2022
View reviewed changes
@skjnldsv skjnldsv force-pushed the feat/theming-default-system-value branch from a8d6143 to 07cf994 Compare October 5, 2022 11:15
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2022

Fixing acceptance...

@skjnldsv skjnldsv force-pushed the feat/theming-default-system-value branch from 07cf994 to 9014a25 Compare October 5, 2022 12:40
@blizzz
Copy link
Member

blizzz commented Oct 5, 2022 •
edited
Loading

also some node trouble

@szaimen
Copy link
Contributor

szaimen commented Oct 5, 2022

Fixing acceptance...

Didnt work completely yet

@szaimen
Copy link
Contributor

szaimen commented Oct 5, 2022

Ill try to test it this evening btw!

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2022 •
edited
Loading

also some node trouble

I didn't compiled to avoid conflicts

Didnt work completely yet

Weird, it passes locally
EDIT/ ah no, it just does not run tests at all...

@skjnldsv skjnldsv force-pushed the feat/theming-default-system-value branch from 9014a25 to 0d8a803 Compare October 5, 2022 16:48
@skjnldsv

This comment was marked as resolved.

Sign in to view
@nextcloud-command nextcloud-command force-pushed the feat/theming-default-system-value branch from 0d8a803 to b8e236e Compare October 5, 2022 17:09
szaimen
szaimen requested changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Seems like the custom color that is delivered with the backgrounds does no longer get correctly applied (the frame around the picture should have a nother color, afaik. The same seems to be true for all other primary elements:
image
image

choosing a custom background, selecting a custom color removed the background again and enables the default theme again.

Also not sure if the custom color should be selectable at all?
image
(don't we already cover all options with the other fileds? At least selecting custom color should not remove any other background picture, no?

@blizzz blizzz dismissed their stale review October 11, 2022 15:21

discussed

@szaimen
Copy link
Contributor

szaimen commented Oct 11, 2022

@skjnldsv #34473 was merged. Please fix conflicts :)

@skjnldsv skjnldsv mentioned this pull request Oct 12, 2022
Closed
This was referenced Oct 12, 2022
Closed
Closed
@szaimen szaimen force-pushed the feat/theming-default-system-value branch from cabe68e to d3933cf Compare October 12, 2022 23:04
@szaimen
Copy link
Contributor

szaimen commented Oct 12, 2022

rebased and fixed the conflicts

CarlSchwan
CarlSchwan requested changes Oct 13, 2022
View reviewed changes
@PVince81 PVince81 mentioned this pull request Oct 13, 2022
Closed
@skjnldsv skjnldsv requested review from CarlSchwan and blizzz October 13, 2022 10:11
@skjnldsv
Use default system primary
4b46c5a 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Allow to pick custom colour
db83135 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Fix primary and debounce to avoid infinite loop
9a92fe9 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

Resolving conflicts

@skjnldsv skjnldsv force-pushed the feat/theming-default-system-value branch from 1c631cd to f85b30b Compare October 13, 2022 10:15
@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 13, 2022
@skjnldsv
Copy link
Member Author

/compile amend /

@szaimen szaimen enabled auto-merge October 13, 2022 14:21
@skjnldsv @nextcloud-command
Adjust testing
2f8601a 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command nextcloud-command force-pushed the feat/theming-default-system-value branch from f85b30b to 2f8601a Compare October 13, 2022 14:31
@PVince81 PVince81 mentioned this pull request Oct 13, 2022
Merged
@PVince81
Copy link
Member

stable25 to save some time hopefully: #34588

@szaimen szaimen merged commit b28757f into master Oct 13, 2022
@szaimen szaimen deleted the feat/theming-default-system-value branch October 13, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@szaimen szaimen szaimen approved these changes

@Pytal Pytal Pytal approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@PVince81 PVince81 Awaiting requested review from PVince81

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@artonge artonge Awaiting requested review from artonge

@blizzz blizzz Awaiting requested review from blizzz

Assignees

@skjnldsv skjnldsv

Labels

4. to release Ready to be released and/or waiting for tests to finish bug feature: theming high regression

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

User theming changes original primary colour

8 participants

@skjnldsv @szaimen @blizzz @Pytal @jancborchardt @PVince81 @CarlSchwan