Merge pull request #43347 from nextcloud/backport/42552/stable28

[stable28] fix(theming): change color button contrast
nextcloud · Feb 6, 2024 · de19878 · de19878
2 parents 341375b + bccad83
commit de19878
Show file tree

Hide file tree

Showing 10 changed files with 68 additions and 70 deletions.
diff --git a/apps/theming/src/AdminTheming.vue b/apps/theming/src/AdminTheming.vue
@@ -51,7 +51,6 @@
 					:default-value="colorPickerField.defaultValue"
 					:display-name="colorPickerField.displayName"
 					:value.sync="colorPickerField.value"
-					data-admin-theming-setting-primary-color
 					@update:theming="$emit('update:theming')" />
 
 				<!-- Default background picker -->

diff --git a/apps/theming/src/components/BackgroundSettings.vue b/apps/theming/src/components/BackgroundSettings.vue
@@ -58,16 +58,15 @@
 		</button>
 
 		<!-- Custom color picker -->
-		<NcColorPicker v-model="Theming.color" @input="debouncePickColor">
-			<button class="background background__color"
-				:data-color="Theming.color"
-				:data-color-bright="invertTextColor(Theming.color)"
-				:style="{ backgroundColor: Theming.color, '--border-color': Theming.color}"
-				data-user-theming-background-color
-				tabindex="0">
-				{{ t('theming', 'Change color') }}
-			</button>
-		</NcColorPicker>
+		<div class="background-color"
+			data-user-theming-background-color>
+			<NcColorPicker v-model="Theming.color"
+				@input="debouncePickColor">
+				<NcButton type="ternary">
+					{{ t('theming', 'Change color') }}
+				</NcButton>
+			</NcColorPicker>
+		</div>
 
 		<!-- Remove background -->
 		<button :aria-pressed="isBackgroundDisabled"
@@ -113,6 +112,7 @@ import { Palette } from 'node-vibrant/lib/color.js'
 import axios from '@nextcloud/axios'
 import debounce from 'debounce'
 import NcColorPicker from '@nextcloud/vue/dist/Components/NcColorPicker.js'
+import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
 import Vibrant from 'node-vibrant'
 
 import Check from 'vue-material-design-icons/Check.vue'
@@ -133,6 +133,7 @@ export default {
 		Check,
 		Close,
 		ImageEdit,
+		NcButton,
 		NcColorPicker,
 	},
 
@@ -341,6 +342,17 @@ export default {
 	flex-wrap: wrap;
 	justify-content: center;
 
+	.background-color {
+		display: flex;
+		justify-content: center;
+		align-items: center;
+		width: 176px;
+		height: 96px;
+		margin: 8px;
+		border-radius: var(--border-radius-large);
+		background-color: var(--color-primary);
+	}
+
 	.background {
 		overflow: hidden;
 		width: 176px;
@@ -368,11 +380,6 @@ export default {
 			border-color: var(--color-border);
 		}
 
-		&__color {
-			color: var(--color-primary-text);
-			background-color: var(--color-primary-default);
-		}
-
 		// Over a background image
 		&__default,
 		&__shipped {

diff --git a/apps/theming/src/components/admin/ColorPickerField.vue b/apps/theming/src/components/admin/ColorPickerField.vue
@@ -26,15 +26,17 @@
 		<div class="field__row">
 			<NcColorPicker :value.sync="localValue"
 				:advanced-fields="true"
+				data-admin-theming-setting-primary-color-picker
 				@update:value="debounceSave">
-				<NcButton class="field__button"
-					type="primary"
-					:id="id"
-					:aria-label="t('theming', 'Select a custom color')"
-					data-admin-theming-setting-primary-color-picker>
-					{{ value }}
+				<NcButton type="secondary"
+					:id="id">
+					<template #icon>
+						<Palette :size="20" />
+					</template>
+					{{ t('theming', 'Change color') }}
 				</NcButton>
 			</NcColorPicker>
+			<div class="field__color-preview" data-admin-theming-setting-primary-color />
 			<NcButton v-if="value !== defaultValue"
 				type="tertiary"
 				:aria-label="t('theming', 'Reset to default')"
@@ -60,6 +62,7 @@ import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
 import NcColorPicker from '@nextcloud/vue/dist/Components/NcColorPicker.js'
 import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js'
 import Undo from 'vue-material-design-icons/UndoVariant.vue'
+import Palette from 'vue-material-design-icons/Palette.vue'
 
 import TextValueMixin from '../../mixins/admin/TextValueMixin.js'
 
@@ -71,6 +74,7 @@ export default {
 		NcColorPicker,
 		NcNoteCard,
 		Undo,
+		Palette,
 	},
 
 	mixins: [
@@ -108,28 +112,10 @@ export default {
 @import './shared/field.scss';
 
 .field {
-	// Override default NcButton styles
-	&__button {
-		width: 230px !important;
-		border-radius: var(--border-radius-large) !important;
-		background-color: var(--color-primary-default) !important;
-
-		// emulated hover state because it would not make sense
-		// to create a dedicated global variable for the color-primary-default
-		&:hover::after {
-			background-color: white;
-			content: "";
-			position: absolute;
-			width: 100%;
-			height: 100%;
-			opacity: .2;
-			filter: var(--primary-invert-if-bright);
-		}
-
-		// Above the ::after
-		&::v-deep * {
-			z-index: 1;
-		}
+	&__color-preview {
+		width: var(--default-clickable-area);
+		border-radius: var(--border-radius-large);
+		background-color: var(--color-primary-default);
 	}
 }
 </style>
diff --git a/cypress/e2e/theming/admin-settings.cy.ts b/cypress/e2e/theming/admin-settings.cy.ts
@@ -41,7 +41,7 @@ describe('Admin theming settings visibility check', function() {
 	})
 
 	it('See the default settings', function() {
-		cy.get('[data-admin-theming-setting-primary-color-picker]').should('contain.text', defaultPrimary)
+		cy.get('[data-admin-theming-setting-primary-color-picker]').should('exist')
 		cy.get('[data-admin-theming-setting-primary-color-reset]').should('not.exist')
 		cy.get('[data-admin-theming-setting-file-reset]').should('not.exist')
 		cy.get('[data-admin-theming-setting-file-remove]').should('be.visible')
@@ -66,8 +66,7 @@ describe('Change the primary color and reset it', function() {
 	it('Change the primary color', function() {
 		cy.intercept('*/apps/theming/ajax/updateStylesheet').as('setColor')
 
-		pickRandomColor('[data-admin-theming-setting-primary-color-picker]')
-			.then(color => { selectedColor = color })
+		pickRandomColor().then(color => { selectedColor = color })
 
 		cy.wait('@setColor')
 		cy.waitUntil(() => validateBodyThemingCss(selectedColor, defaultBackground))
@@ -151,8 +150,7 @@ describe('Remove the default background with a custom primary color', function()
 	it('Change the primary color', function() {
 		cy.intercept('*/apps/theming/ajax/updateStylesheet').as('setColor')
 
-		pickRandomColor('[data-admin-theming-setting-primary-color-picker]')
-			.then((color) => { selectedColor = color })
+		pickRandomColor().then(color => { selectedColor = color })
 
 		cy.wait('@setColor')
 		cy.waitUntil(() => validateBodyThemingCss(selectedColor, defaultBackground))
@@ -372,8 +370,7 @@ describe('The user default background settings reflect the admin theming setting
 	it('Change the primary color', function() {
 		cy.intercept('*/apps/theming/ajax/updateStylesheet').as('setColor')
 
-		pickRandomColor('[data-admin-theming-setting-primary-color-picker]')
-			.then(color => { selectedColor = color })
+		pickRandomColor().then(color => { selectedColor = color })
 
 		cy.wait('@setColor')
 		cy.waitUntil(() => cy.window().then(($window) => {

diff --git a/cypress/e2e/theming/themingUtils.ts b/cypress/e2e/theming/themingUtils.ts
@@ -54,13 +54,12 @@ export const validateBodyThemingCss = function(expectedColor = defaultPrimary, e
  */
 export const validateUserThemingDefaultCss = function(expectedColor = defaultPrimary, expectedBackground: string|null = defaultBackground) {
 	const defaultSelectButton = Cypress.$('[data-user-theming-background-default]')
-	const customColorSelectButton = Cypress.$('[data-user-theming-background-color]')
-	if (defaultSelectButton.length === 0 || customColorSelectButton.length === 0) {
+	if (defaultSelectButton.length === 0) {
 		return false
 	}
 
 	const defaultOptionBackground = defaultSelectButton.css('background-image')
-	const colorPickerOptionColor = customColorSelectButton.css('background-color')
+	const colorPickerOptionColor = defaultSelectButton.css('background-color')
 
 	const isValidBackgroundImage = !expectedBackground
 		? defaultOptionBackground === 'none'
@@ -71,16 +70,26 @@ export const validateUserThemingDefaultCss = function(expectedColor = defaultPri
 	return isValidBackgroundImage && colord(colorPickerOptionColor).isEqual(expectedColor)
 }
 
-export const pickRandomColor = function(pickerSelector: string): Cypress.Chainable<string> {
+export const pickRandomColor = function(): Cypress.Chainable<string> {
 	// Pick one of the first 8 options
 	const randColour = Math.floor(Math.random() * 8)
 
-	// Open picker
-	cy.get(pickerSelector).click()
+	const colorPreviewSelector = '[data-user-theming-background-color],[data-admin-theming-setting-primary-color]'
 
-	// Return selected colour
-	return cy.get(pickerSelector).get('.color-picker__simple-color-circle').eq(randColour).then(($el) => {
-		$el.trigger('click')
-		return $el.css('background-color')
+	let oldColor = ''
+	cy.get(colorPreviewSelector).then(($el) => {
+		oldColor = $el.css('background-color')
 	})
+
+	// Open picker
+	cy.contains('button', 'Change color').click()
+
+	// Click on random color
+	cy.get('.color-picker__simple-color-circle').eq(randColour).click()
+
+	// Wait for color change
+	cy.waitUntil(() => Cypress.$(colorPreviewSelector).css('background-color') !== oldColor)
+
+	// Get the selected color from the color preview block
+	return cy.get(colorPreviewSelector).then(($el) => $el.css('background-color'))
 }
diff --git a/cypress/e2e/theming/user-background.cy.ts b/cypress/e2e/theming/user-background.cy.ts
@@ -129,7 +129,7 @@ describe('User select a custom color', function() {
 	it('Select a custom color', function() {
 		cy.intercept('*/apps/theming/background/color').as('setColor')
 
-		pickRandomColor('[data-user-theming-background-color]')
+		pickRandomColor()
 
 		// Validate custom colour change
 		cy.wait('@setColor')
@@ -168,7 +168,7 @@ describe('User select a bright custom color and remove background', function() {
 		cy.intercept('*/apps/theming/background/color').as('setColor')
 
 		// Pick one of the bright color preset
-		cy.get('[data-user-theming-background-color]').click()
+		cy.contains('button', 'Change color').click()
 		cy.get('.color-picker__simple-color-circle:eq(4)').click()
 
 		// Validate custom colour change
@@ -286,7 +286,7 @@ describe('User changes settings and reload the page', function() {
 	it('Select a custom color', function() {
 		cy.intercept('*/apps/theming/background/color').as('setColor')
 
-		cy.get('[data-user-theming-background-color]').click()
+		cy.contains('button', 'Change color').click()
 		cy.get('.color-picker__simple-color-circle:eq(5)').click()
 
 		// Validate clear background

diff --git a/dist/theming-admin-theming.js b/dist/theming-admin-theming.js
diff --git a/dist/theming-admin-theming.js.map b/dist/theming-admin-theming.js.map
diff --git a/dist/theming-personal-theming.js b/dist/theming-personal-theming.js
diff --git a/dist/theming-personal-theming.js.map b/dist/theming-personal-theming.js.map