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.

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

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

Merged
merged 1 commit into from
Mar 2, 2023
Merged

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

merged 1 commit into from
Mar 2, 2023

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 artonge, Pytal 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

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

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
fix(theming): select "no-background" option and fix layout order
162cc3c 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@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
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
4 participants
@skjnldsv @szaimen @artonge @Pytal