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.

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

Finalize TwG setting validation #5524

Closed
felixarntz opened this issue Jul 8, 2022 · 8 comments
Closed

Finalize TwG setting validation #5524

felixarntz opened this issue Jul 8, 2022 · 8 comments
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Jul 8, 2022

Follow-up to #5461: We need to allow for all possible color themes to be selectable within the client-side TwG setting validation.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • There should be a utility function that returns the data for the TwG color theme options, each one including the identifier, human-readable name (translatable), and SVG to use in the UI. The options should come from this Figma design.
  • The JS setting validation for the TwG colorTheme setting should be enhanced to allow for any of the identifiers from the above list of colors to be chosen (instead of the currently hard-coded "blue").
  • The TwG SettingsView UI should also be adjusted accordingly to show human-readable color names: For displaying the colorTheme value, it should no longer just display the identifier, but rather the human-readable name for the identifier set.

Implementation Brief

Note: Before you begin implementation, make sure the color list and names are finalized (see this Figma comment).

In assets/js/modules/thank-with-google/util/settings.js:

  • Create and export a new function called getColorThemes.
  • This should return a list of objects each with properties colorThemeID, name, and svg.
  • The color list should be as follows (double check this with the Figma file):
    • id: blue, name: Blue
    • id: cyan, name: Cyan
    • id: green, name: Green
    • id: purple, name: Purple
    • id: pink, name: Pink
    • id: orange, name: Orange
    • id: brown, name: Brown
    • id: black, name: Black
  • Export the corresponding SVGs from Figma and include them in the objects.
  • Don't forget to translate the names.

In assets/js/modules/thank-with-google/util/validation.js:

  • Update the isValidColorTheme function to retrieve the list of themes from the new getColorThemes function and ensure the passed colorTheme matches a colorThemeID in the list of objects.

In assets/js/modules/thank-with-google/components/settings/SettingsView.js:

  • Retrieve the list of themes via getColorThemes and use this for rendering the color theme options.

Test Coverage

  • Add jest tests for the validation function.

QA Brief

  • Check the storybook story for the settings view component and verify that you see the colour settings there.

Changelog entry

  • Show selected Thank with Google color in settings view.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues labels Jul 8, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Jul 8, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Jul 11, 2022
@eugene-manuilov eugene-manuilov self-assigned this Jul 14, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov
Copy link
Collaborator

@techanvil fyi I have added a note to write jest tests for the validation function.

@eugene-manuilov eugene-manuilov removed their assignment Jul 14, 2022
@techanvil
Copy link
Collaborator

@techanvil fyi I have added a note to write jest tests for the validation function.

Nice one @eugene-manuilov!

@wpdarren wpdarren self-assigned this Jul 26, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jul 26, 2022

QA Update: ⚠️

@eugene-manuilov In the QAB it mentions the Settings view component on Storybook. I found this but I am unsure how to compare this to figma designs. Can you clarify what I should be checking on this? I feel the QAB needs more details, unless I am missing something obvious.

@kuasha420
Copy link
Contributor

kuasha420 commented Jul 26, 2022

QA: ✖️

Due the serverside validation added here in the #5461, Ttrying to save anything other than blue color will result an empty string.

@eugene-manuilov
Copy link
Collaborator

QA: ✖️

Due the serverside validation added here in the #5461, Ttrying to save anything other than blue color will result an empty string.

Good catch! Thanks, @kuasha420. Added a follow up PR that fixes it: #5610.

@eugene-manuilov
Copy link
Collaborator

QA Update: ⚠️

@eugene-manuilov In the QAB it mentions the Settings view component on Storybook. I found this but I am unsure how to compare this to figma designs. Can you clarify what I should be checking on this? I feel the QAB needs more details, unless I am missing something obvious.

@wpdarren UI changes in the settings view component are out of scope for this ticket. The intention here is to make sure that the colour setting shows up on the settings view now.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • To test this I used code to connect TwG (via the functions.php file) and then changed the color ID in the code as per the AC. Then in the TwG settings, I made sure that the title case version of the color appeared:

    • id: blue, name: Blue
    • id: cyan, name: Cyan
    • id: green, name: Green
    • id: purple, name: Purple
    • id: pink, name: Pink
    • id: orange, name: Orange
    • id: brown, name: Brown
    • id: black, name: Black

image

function democode_return_completed_twg_configuration(  ) {
  return array(
      'publicationID' => 'TEST-PUBLICATION-ID',
      'colorTheme' => 'blue',
      'buttonPlacement' => 'dynamic_low',
      'buttonPostTypes' => array( 'post' ),
  );
}

@wpdarren wpdarren removed their assignment Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants