Skip to content

fix(theming): select "no-background" option and fix layout order #36931

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
skjnldsv merged 1 commit into from
Mar 2, 2023
Merged

fix(theming): select "no-background" option and fix layout order #36931

skjnldsv merged 1 commit into from
Mar 2, 2023
+31 −20

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Mar 1, 2023

Fix #35479

No background Custom background
2023-03-01_09-45 2023-03-01_09-45_1
Shipped background Default background
2023-03-01_09-45_2 2023-03-01_09-45_3
  • Renamed "Remove background" to "No background" to better reflect that this is an option and not just an action
  • Make the "Remove background" reflect the user choice
  • Moved the "Remove background" option in the first row to better separate the shipped backgrounds
  • Added an icon on the "Custom background" option

@skjnldsv skjnldsv added this to the Nextcloud 26 milestone Mar 1, 2023
@skjnldsv skjnldsv requested a review from a team March 1, 2023 08:53
@skjnldsv skjnldsv self-assigned this Mar 1, 2023
@skjnldsv skjnldsv requested review from Pytal, artonge and szaimen and removed request for a team March 1, 2023 08:53
artonge
artonge reviewed Mar 1, 2023
View reviewed changes
apps/theming/src/components/BackgroundSettings.vue
@click="removeBackground">
{{ t('theming', 'No background') }}
<Close v-if="!isBackgroundDisabled" :size="32" />
<Check :size="44" />
Copy link
Contributor

@artonge artonge Mar 1, 2023

Choose a reason for hiding this comment

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

Any reason you used css instead v-else ?

Copy link
Member Author

@skjnldsv skjnldsv Mar 1, 2023

Choose a reason for hiding this comment

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

I don't recall 🤷
I think it was like that before, and I left it when I switched from icon-confirm to the Check vue component
This is not related tot his PR anyway :)

@skjnldsv skjnldsv requested a review from artonge March 1, 2023 10:14
szaimen
szaimen approved these changes Mar 1, 2023
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.

Works in my testing 🚀

@szaimen
Copy link
Contributor

szaimen commented Mar 1, 2023

But conflicts and jsunit fails...

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 2, 2023
@skjnldsv skjnldsv force-pushed the fix/theming-background branch from 92f8283 to 162cc3c Compare March 2, 2023 06:22
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 2, 2023

and jsunit fails

I don't think it's related

@skjnldsv skjnldsv merged commit e6aca96 into master Mar 2, 2023
@skjnldsv skjnldsv deleted the fix/theming-background branch March 2, 2023 06:50
@blizzz blizzz mentioned this pull request Mar 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@Pytal Pytal Pytal approved these changes

@szaimen szaimen szaimen approved these changes

Assignees

@skjnldsv skjnldsv

Labels

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

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

Theming refactoring follow-ups

5 participants

@skjnldsv @szaimen @artonge @Pytal