From db288c0b4b37c0ed500492bfb5dfdc4c168bf7fd Mon Sep 17 00:00:00 2001 From: Suzanne Aitchison Date: Thu, 3 Mar 2022 08:22:10 +0000 Subject: [PATCH] Use the Preact ColorPicker in creator onboarding (#16731) * use Preact ColorPicker in creator onboarding * update tests * update comments * Update app/javascript/packs/admin/creatorOnboarding.jsx Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> --- .../creator_settings_controller.js | 67 ------------------- .../replaceTextInputWithColorPicker.jsx} | 20 ++++-- .../formElements/ColorPicker/ColorPicker.jsx | 43 ++++++++++-- .../__tests__/ColorPicker.test.jsx | 22 +++++- .../__snapshots__/ColorPicker.test.jsx.snap | 4 +- .../packs/admin/creatorOnboarding.jsx | 65 ++++++++++++++++++ app/javascript/packs/base.jsx | 8 +-- app/javascript/packs/enhanceColorPickers.jsx | 9 +++ .../utilities/color/accentCalculator.js | 5 ++ .../admin/creator_settings/_form.html.erb | 14 ++-- app/views/admin/creator_settings/new.html.erb | 2 +- app/views/shared/_logo_design.html.erb | 2 +- app/views/tags/edit.html.erb | 2 +- app/views/users/_profile.html.erb | 2 +- .../creatorSettings.spec.js | 20 +++--- 15 files changed, 174 insertions(+), 111 deletions(-) delete mode 100644 app/javascript/admin/controllers/creator_settings_controller.js rename app/javascript/{packs/colorPicker.jsx => colorPickers/replaceTextInputWithColorPicker.jsx} (65%) create mode 100644 app/javascript/packs/admin/creatorOnboarding.jsx create mode 100644 app/javascript/packs/enhanceColorPickers.jsx diff --git a/app/javascript/admin/controllers/creator_settings_controller.js b/app/javascript/admin/controllers/creator_settings_controller.js deleted file mode 100644 index 5f5928636..000000000 --- a/app/javascript/admin/controllers/creator_settings_controller.js +++ /dev/null @@ -1,67 +0,0 @@ -import { Controller } from '@hotwired/stimulus'; -import { isLowContrast } from '@utilities/color/contrastValidator'; -import { brightness } from '@utilities/color/accentCalculator'; - -/** - * Manages interactions on the Creator Settings page. - */ -export class CreatorSettingsController extends Controller { - static targets = ['colorContrastError', 'brandColor']; - - /** - * Validates the color contrast for accessibility, - * if the contrast is okay, it updates the branding, - * else it displays the error. - * @param {Event} event - */ - handleValidationsAndUpdates(event) { - const { value: color } = event.target; - - if (isLowContrast(color)) { - this.colorContrastErrorTarget.innerText = - 'The selected color must be darker for accessibility purposes.'; - } else { - this.updateBranding(color); - this.colorContrastErrorTarget.innerText = ''; - } - } - - /** - * Updates ths branding/colors on the Creator Settings Page. - * by overriding the accent-color in the :root object - * - * @param {String} color - */ - updateBranding(color) { - if (!new RegExp(event.target.getAttribute('pattern')).test(color)) { - return; - } - - document.documentElement.style.setProperty('--accent-brand', color); - - // We need to recalculate '--accent-brand-darker' in javascript as it's - // currently being calculated in ruby. It is used for the hover effect - // over the button. - // 0.85 represents the brightness value set in Ruby to calculate - // '--accent-brand-darker' - document.documentElement.style.setProperty( - '--accent-brand-darker', - brightness(color, 0.85), - ); - } - - /** - * Prevents a submission of the form if the - * color contrast is low. - * - * @param {Event} event - */ - formValidations(event) { - const { value: color } = this.brandColorTarget; - if (isLowContrast(color)) { - event.preventDefault(); - this.colorContrastErrorTarget.classList.remove('hidden'); - // we don't want the form to submit if the contrast is low - } - } -} diff --git a/app/javascript/packs/colorPicker.jsx b/app/javascript/colorPickers/replaceTextInputWithColorPicker.jsx similarity index 65% rename from app/javascript/packs/colorPicker.jsx rename to app/javascript/colorPickers/replaceTextInputWithColorPicker.jsx index 03bd8ae10..37cbc7675 100644 --- a/app/javascript/packs/colorPicker.jsx +++ b/app/javascript/colorPickers/replaceTextInputWithColorPicker.jsx @@ -1,11 +1,18 @@ import { h, render } from 'preact'; import { ColorPicker } from '@crayons'; -// Find any color picker inputs on the page and replace them with the Preact enhanced component -const colorInputs = document.querySelectorAll('[data-color-picker]'); - -for (const input of colorInputs) { - const { labelText } = input.dataset; +/** + * Takes a text input, and replaces it with the richer Preact component + * + * @param {HTMLElement} input The input to replace + * @param {string} labelText The label to apply to the new form controls + * @param {function} onChange Any onChange callback + */ +export function replaceTextInputWithColorPicker({ + input, + labelText, + onChange, +}) { const inputProps = {}; // Copy any specific attributes to the new input @@ -15,13 +22,14 @@ for (const input of colorInputs) { } // The third `replaceNode` argument here makes sure that the new picker is rendered in the correct position within the parentElement - // However, Preact is unable to do a VDOM diff that allows a straight replacement of one input for the other, so we also need to remove it manually on line 29 + // However, Preact is unable to do a VDOM diff that allows a straight replacement of one input for the other, so we also need to remove it manually below render( , input.parentElement, input, diff --git a/app/javascript/crayons/formElements/ColorPicker/ColorPicker.jsx b/app/javascript/crayons/formElements/ColorPicker/ColorPicker.jsx index dba6d2ebb..33056598d 100644 --- a/app/javascript/crayons/formElements/ColorPicker/ColorPicker.jsx +++ b/app/javascript/crayons/formElements/ColorPicker/ColorPicker.jsx @@ -6,11 +6,21 @@ import { HexColorPicker, HexColorInput } from 'react-colorful'; import { initializeDropdown } from '@utilities/dropdownUtils'; import { ButtonNew as Button } from '@crayons'; +const convertThreeCharHexToSix = (hex) => { + const r = hex.charAt(1); + const g = hex.charAt(2); + const b = hex.charAt(3); + + return `#${r}${r}${g}${g}${b}${b}`; +}; + export const ColorPicker = ({ id, buttonLabelText, defaultValue, inputProps, + onChange, + onBlur, }) => { // Ternary has been used here to guard against an empty string being passed as default value const [color, setColor] = useState(defaultValue ? defaultValue : '#000'); @@ -25,9 +35,21 @@ export const ColorPicker = ({ }); }, [buttonId, popoverId]); + // Hex codes may validly be represented by three characters, where r, g, b are all repeated, + // e.g. #0D6 === #00DD66. To make sure that all color codes can be handled consistently through our app, + // we convert any shorthand hex codes to their full 6 char representation. + const handleBlur = () => { + // Color always includes a leading '#', hence a length of 4 + if (color.length === 4) { + const fullHexCode = convertThreeCharHexToSix(color); + setColor(fullHexCode); + onChange?.(fullHexCode); + } + }; + return ( -
+
diff --git a/app/javascript/crayons/formElements/ColorPicker/__tests__/ColorPicker.test.jsx b/app/javascript/crayons/formElements/ColorPicker/__tests__/ColorPicker.test.jsx index 3eaf6c43e..cc4217637 100644 --- a/app/javascript/crayons/formElements/ColorPicker/__tests__/ColorPicker.test.jsx +++ b/app/javascript/crayons/formElements/ColorPicker/__tests__/ColorPicker.test.jsx @@ -1,5 +1,5 @@ import { h } from 'preact'; -import { render } from '@testing-library/preact'; +import { render, waitFor } from '@testing-library/preact'; import { axe } from 'jest-axe'; import { ColorPicker } from '@crayons'; @@ -46,4 +46,24 @@ describe('', () => { ); expect(container.innerHTML).toMatchSnapshot(); }); + + it('converts 3 char hex codes to full 6 chars on blur', async () => { + const changeHandler = jest.fn(); + + const { getByRole } = render( + , + ); + + const input = getByRole('textbox', { name: 'Choose a color' }); + input.focus(); + input.blur(); + + await waitFor(() => expect(changeHandler).toHaveBeenCalledWith('#00BB66')); + }); }); diff --git a/app/javascript/crayons/formElements/ColorPicker/__tests__/__snapshots__/ColorPicker.test.jsx.snap b/app/javascript/crayons/formElements/ColorPicker/__tests__/__snapshots__/ColorPicker.test.jsx.snap index 577962be8..1e7d48afb 100644 --- a/app/javascript/crayons/formElements/ColorPicker/__tests__/__snapshots__/ColorPicker.test.jsx.snap +++ b/app/javascript/crayons/formElements/ColorPicker/__tests__/__snapshots__/ColorPicker.test.jsx.snap @@ -1,5 +1,5 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[` should render 1`] = `"
"`; +exports[` should render 1`] = `"
"`; -exports[` should render with a default value 1`] = `"
"`; +exports[` should render with a default value 1`] = `"
"`; diff --git a/app/javascript/packs/admin/creatorOnboarding.jsx b/app/javascript/packs/admin/creatorOnboarding.jsx new file mode 100644 index 000000000..7c8673b21 --- /dev/null +++ b/app/javascript/packs/admin/creatorOnboarding.jsx @@ -0,0 +1,65 @@ +import { replaceTextInputWithColorPicker } from '../../colorPickers/replaceTextInputWithColorPicker'; +import { isLowContrast } from '@utilities/color/contrastValidator'; +import { brightness } from '@utilities/color/accentCalculator'; + +const vanillaPicker = document.getElementById( + 'creator_settings_form_primary_brand_color_hex', +); +const contrastErrorMessage = document.getElementById('color-contrast-error'); +const finishButton = document.getElementById('finish-button'); + +if (vanillaPicker) { + replaceTextInputWithColorPicker({ + input: vanillaPicker, + labelText: 'Brand color', + onChange: handleValidationsAndUpdates, + }); + + // We don't want the form to submit if the contrast is too low + finishButton.addEventListener('click', (event) => { + const { value: color } = document.getElementById( + 'creator_settings_form_primary_brand_color_hex', + ); + + if (isLowContrast(color)) { + event.preventDefault(); + } + }); +} + +/** + * Validates the color contrast for accessibility, + * if the contrast is okay, it updates the branding, + * else it displays the error. + * + * @param {string} color The color hex code + */ +function handleValidationsAndUpdates(color) { + if (isLowContrast(color)) { + contrastErrorMessage.innerText = + 'The selected color must be darker for accessibility purposes.'; + } else { + updateBranding(color); + contrastErrorMessage.innerText = ''; + } +} + +/** + * Updates the branding/colors on the Creator Settings Page + * by overriding the accent-color in the :root object + * + * @param {String} color The color hex code + */ +function updateBranding(color) { + document.documentElement.style.setProperty('--accent-brand', color); + + // We need to recalculate '--accent-brand-darker' in javascript as it's + // currently being calculated in ruby. It is used for the hover effect + // over the button. + // 0.85 represents the brightness value set in Ruby to calculate + // '--accent-brand-darker' + document.documentElement.style.setProperty( + '--accent-brand-darker', + brightness(color, 0.85), + ); +} diff --git a/app/javascript/packs/base.jsx b/app/javascript/packs/base.jsx index 5ff84453f..8f3640f8b 100644 --- a/app/javascript/packs/base.jsx +++ b/app/javascript/packs/base.jsx @@ -156,18 +156,12 @@ initializeNav(); async function loadCreatorSettings() { try { - const [ - { CreatorSettingsController }, - { LogoUploadController }, - { Application }, - ] = await Promise.all([ - import('@admin/controllers/creator_settings_controller'), + const [{ LogoUploadController }, { Application }] = await Promise.all([ import('@admin/controllers/logo_upload_controller'), import('@hotwired/stimulus'), ]); const application = Application.start(); - application.register('creator-settings', CreatorSettingsController); application.register('logo-upload', LogoUploadController); } catch (error) { Honeybadger.notify( diff --git a/app/javascript/packs/enhanceColorPickers.jsx b/app/javascript/packs/enhanceColorPickers.jsx new file mode 100644 index 000000000..3595312b6 --- /dev/null +++ b/app/javascript/packs/enhanceColorPickers.jsx @@ -0,0 +1,9 @@ +import { replaceTextInputWithColorPicker } from '../colorPickers/replaceTextInputWithColorPicker'; + +// Find any color picker inputs on the page and replace them with the Preact enhanced component +const colorInputs = document.querySelectorAll('[data-color-picker]'); + +for (const input of colorInputs) { + const { labelText } = input.dataset; + replaceTextInputWithColorPicker({ input, labelText }); +} diff --git a/app/javascript/utilities/color/accentCalculator.js b/app/javascript/utilities/color/accentCalculator.js index 0e1a5f053..ceca3691c 100644 --- a/app/javascript/utilities/color/accentCalculator.js +++ b/app/javascript/utilities/color/accentCalculator.js @@ -7,6 +7,11 @@ */ export function brightness(color, amount = 1) { const rgbObj = hexToRgb(color); + + if (!rgbObj) { + return null; + } + Object.keys(rgbObj).forEach((key) => { rgbObj[key] = Math.round(rgbObj[key] * amount); }); diff --git a/app/views/admin/creator_settings/_form.html.erb b/app/views/admin/creator_settings/_form.html.erb index 51f784293..9e76d95b5 100644 --- a/app/views/admin/creator_settings/_form.html.erb +++ b/app/views/admin/creator_settings/_form.html.erb @@ -22,19 +22,12 @@ pattern: "^#+([a-fA-F0-9]{6})$", title: "Provide a valid HEX Color or pick your color from the color picker.", placeholder: ::Settings::UserExperience.primary_brand_color_hex, - class: "crayons-textfield js-color-field", - "data-action": "change->creator-settings#handleValidationsAndUpdates", - "data-creator-settings-target": "brandColor", + class: "crayons-textfield", "aria-describedby": "color-contrast-error" %> - <%= f.color_field :primary_brand_color_hex, - pattern: "^#+([a-fA-F0-9]{6})$", - placeholder: ::Settings::UserExperience.primary_brand_color_hex, - class: "crayons-color-selector js-color-field ml-2", - required: true, - "data-action": "change->creator-settings#handleValidationsAndUpdates" %> +
-
+
@@ -89,3 +82,4 @@
+<%= javascript_packs_with_chunks_tag "admin/creatorOnboarding", defer: true %> diff --git a/app/views/admin/creator_settings/new.html.erb b/app/views/admin/creator_settings/new.html.erb index 9cea455c0..a487fdc90 100644 --- a/app/views/admin/creator_settings/new.html.erb +++ b/app/views/admin/creator_settings/new.html.erb @@ -22,7 +22,7 @@
<%= render "form", creator_settings_form: @creator_settings_form, f: f %> -
+
<%= f.submit "Finish", class: "crayons-btn btn--primary" %>
<% if @help_url %> diff --git a/app/views/shared/_logo_design.html.erb b/app/views/shared/_logo_design.html.erb index 03db185bf..3f9b4187a 100644 --- a/app/views/shared/_logo_design.html.erb +++ b/app/views/shared/_logo_design.html.erb @@ -34,4 +34,4 @@ <%= inline_svg_tag("preview-logo.svg", id: "color-select-preview-logo", class: "p-4 radius-default", title: t("views.logo.preview.icon")) %>
-<%= javascript_packs_with_chunks_tag "colorPicker", defer: true %> +<%= javascript_packs_with_chunks_tag "enhanceColorPickers", defer: true %> diff --git a/app/views/tags/edit.html.erb b/app/views/tags/edit.html.erb index f1e678d84..7645d27a3 100644 --- a/app/views/tags/edit.html.erb +++ b/app/views/tags/edit.html.erb @@ -56,4 +56,4 @@ <% end %> -<%= javascript_packs_with_chunks_tag "colorPicker", defer: true %> +<%= javascript_packs_with_chunks_tag "enhanceColorPickers", defer: true %> diff --git a/app/views/users/_profile.html.erb b/app/views/users/_profile.html.erb index 9ef743cfd..32570c65a 100644 --- a/app/views/users/_profile.html.erb +++ b/app/views/users/_profile.html.erb @@ -1,4 +1,4 @@ -<%= javascript_packs_with_chunks_tag "colorPicker", "stickySaveFooter", "userProfileSettings", "validateFileInputs", defer: true %> +<%= javascript_packs_with_chunks_tag "enhanceColorPickers", "stickySaveFooter", "userProfileSettings", "validateFileInputs", defer: true %> <%= render "users/additional_authentication" %> diff --git a/cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js b/cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js index 299bd8bbe..1468628f7 100644 --- a/cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js +++ b/cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js @@ -33,9 +33,11 @@ describe('Creator Settings Page', () => { 'be.visible', ); - // should contain a brand color field - cy.findByText(/^Brand color/).should('be.visible'); - cy.findByText(/^Brand color/).invoke('attr', 'value', '#ff0000'); + // should contain a brand color field, enhanced with popover picker + cy.findByRole('button', { name: /^Brand color/ }).should('be.visible'); + cy.findByRole('textbox', { name: /^Brand color/ }) + .clear() + .type('#BC1A90'); // should contain a 'Who can join this community?' radio selector field and allow selection upon click cy.findByRole('group', { name: /^Who can join this community/i }) @@ -100,7 +102,7 @@ describe('Creator Settings Page', () => { it('should show an error when the contrast ratio of a brand color is too low', () => { const lowContrastColor = '#a6e8a6'; - cy.findByLabelText(/^Brand color/) + cy.findByRole('textbox', { name: /^Brand color/ }) .clear() .type(lowContrastColor) .blur(); @@ -113,7 +115,7 @@ describe('Creator Settings Page', () => { it('should not show an error when the contrast ratio of a brand color is good', () => { const adequateContrastColor = '#25544b'; - cy.findByLabelText(/^Brand color/) + cy.findByRole('textbox', { name: /^Brand color/ }) .clear() .type(adequateContrastColor) .blur(); @@ -129,7 +131,7 @@ describe('Creator Settings Page', () => { const lowContrastColor = '#a6e8a6'; const lowContrastRgbColor = 'rgb(166, 232, 166)'; - cy.findByLabelText(/^Brand color/) + cy.findByRole('textbox', { name: /^Brand color/ }) .clear() .type(lowContrastColor) .blur(); @@ -149,7 +151,7 @@ describe('Creator Settings Page', () => { const color = '#25544b'; const rgbColor = 'rgb(37, 84, 75)'; - cy.findByLabelText(/^Brand color/) + cy.findByRole('textbox', { name: /^Brand color/ }) .clear() .type(color) .blur(); @@ -234,9 +236,7 @@ describe('Admin -> Customization -> Config -> Images', () => { 'not.exist', ); - cy.findAllByRole('img', { name: /DEV\(local\)/i }).should( - 'not.exist', - ) + cy.findAllByRole('img', { name: /DEV\(local\)/i }).should('not.exist'); // we should see the community name instead of a logo cy.get('.site-logo__community-name')