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

SHS-5783 / SHS-5784: New Well Color for Collections #1602

Merged
merged 24 commits into from
Sep 26, 2024

Conversation

codechefmarc
Copy link
Collaborator

@codechefmarc codechefmarc commented Aug 15, 2024

READY FOR REVIEW

Summary

Backend

  • Adds two new fields: Background Color (field_bg_color) and Background Color Width (field_bg_color_width) to the Collection and Private Collection paragraphs
  • Background Color Width only appears if Background Color has a value
  • Hides the old Background Color field (field_paragraph_style) - to be cleaned up in a subsequent PR.
  • As part of a deploy hook, if the old Background Color field had a value, marks the new Background Color field with the default value of "Color Palette Default"

Frontend

Update the styles and logic to make the new two fields Background Color (field_bg_color) and Background Color Width (field_bg_color_width) work in the FE

Need Review By (Date)

09/11/2024

Urgency

medium

Steps to Test

Backend

  1. If using the Tugboat site to test, the deploy hook should have already been run
  2. Edit an existing page that had a background color set (for hs_colorful a good example is "Collection with Mixed Components in a Well"
  3. Open the "Collection" paragraph in the Main Region
  4. Verify that the Background Color field has a value of "Color Palette Default"
  5. Verify that the Background Color Width field is only visible when the Background color field is set
Screenshot 2024-09-16 at 2 25 25 PM
  1. Verify that if "- None -" is selected for the Background Color that the Background Color Width field disappears
  2. Verify that the old Background Color field (field_paragraph_style) (now labeled "Background Color (old)" for Collections and "Style (old)" for Private Collections) is not visible at all on the Paragraph form. (Verify via /admin/structure/paragraphs_type/hs_collection/form-display for Collections and /admin/structure/paragraphs_type/hs_priv_collection/form-display for Private Collections)

Screenshot 2024-08-14 at 5 31 25 PM

  1. Verify that the correct options are available for the Background Color field: Default, Grey, Light and that this field is optional
Screenshot 2024-09-16 at 2 25 35 PM
  1. Verify that the correct options are available for the Background Color Width field: Limited Width, Full Width and that this field (when visible) is required.
Screenshot 2024-09-16 at 2 26 17 PM
  1. Repeat the previous tests in a Private Page with a Private Collection component.

Frontend

  1. On Traditional and Colorful
  2. Create a Flexible Page and add Collections for each Background Color and each Background Color Width
  3. Verify the width and the color is correct in all different color pairings. It should look like what it's proposed in the design (Traditional / Colorful). The distribution of the colors in the design it's in the following way:
  • Color default value -> Colors in the first column (left side of the page)
  • Grey -> Color in the second column (center of the page)
  • Light -> Colors in the last column (right side of the page)
  1. Please, also verify that existing Collection components that had a background set the well color looks as expected
  2. Repeat the previous tests in a Private Page with a Private Collection component.

PR Checklist


@ahughes3 ahughes3 temporarily deployed to Tugboat August 15, 2024 00:29 Destroyed
@codechefmarc codechefmarc self-assigned this Aug 15, 2024
@codechefmarc codechefmarc marked this pull request as ready for review August 15, 2024 04:07
@dalin- dalin- removed their request for review August 15, 2024 13:29
Copy link
Collaborator

@dalin- dalin- left a comment

Choose a reason for hiding this comment

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

I thought I'd do a review since @cienvaras is out until Monday. This code all looks great. Your explanation matches what I intended. But I'm not sure how to log into tugboat to do an extra confirmation.

What do you suggest for how @mariannuar might move forward tomorrow before Andrés is back? Continue working on this branch?

@codechefmarc
Copy link
Collaborator Author

Thanks! I sent the login info to you in Slack. As to FE work - I think that's fine to work on this same branch. I saw the note about possible changes to the color names, though, so perhaps wait a bit before those are finalized. Then I can update the BE code to match those new colors (if any change) and then FE work can start.

@mariannuar
Copy link
Collaborator

@dalin- (cc: @codechefmarc ) Just a heads up that I'm done with my allocations this week for this project :) and I'm currently working on the multiselect ticket, so I think I could start working on the FE part of this on Monday afternoon. Thank you!

Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc Need to change a bit how the new field values are being updated. Please take a look at the inline comment and let me know if you have any questions.

@ahughes3 ahughes3 temporarily deployed to Tugboat August 20, 2024 17:29 Destroyed
@codechefmarc codechefmarc requested a review from cienvaras August 20, 2024 18:33
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc Thanks for implementing the update hook. Code looks good, just found an issue when setting one of the new values, but should be easy to fix.

Base automatically changed from 11.2.2-release to develop August 21, 2024 15:28
@ahughes3 ahughes3 temporarily deployed to Tugboat August 21, 2024 17:32 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat August 21, 2024 18:20 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat August 21, 2024 19:52 Destroyed
Copy link
Collaborator

@dalin- dalin- left a comment

Choose a reason for hiding this comment

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

Some suggestions to get a better complexity score.

$sandbox['total'] = count($sandbox['ids']);
}

$paragraph_ids = array_splice($sandbox['ids'], 0, 10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to complexity, but this technique is not safe. If an exception is thrown on item 5, the rest of the items in this loop will be skipped. Better to wait to remove an item until after it's been successfully processed.

@codechefmarc codechefmarc requested a review from cienvaras August 21, 2024 21:00
@codechefmarc
Copy link
Collaborator Author

@cienvaras - Ok, this is ready for a re-review. Thank you for the find on setting the second field, that is now fixed.

@cienvaras cienvaras changed the base branch from develop to 11.2.4-release September 4, 2024 23:23
@cienvaras
Copy link
Collaborator

@ahughes3 Fixed, both the field order and the screenshots. Let us know if any other change is required.

@ahughes3 ahughes3 temporarily deployed to Tugboat September 16, 2024 20:32 Destroyed
Base automatically changed from 11.2.4-release to develop September 18, 2024 15:31
@ahughes3 ahughes3 temporarily deployed to Tugboat September 19, 2024 18:05 Destroyed
@cienvaras cienvaras changed the base branch from develop to 11.2.5-release September 19, 2024 18:05
@ahughes3 ahughes3 requested a review from joegl September 20, 2024 19:25
Copy link
Contributor

@joegl joegl left a comment

Choose a reason for hiding this comment

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

A couple of recommendations and suggestions.

Copy link
Collaborator

@ahughes3 ahughes3 left a comment

Choose a reason for hiding this comment

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

Looks good!

@ahughes3 ahughes3 assigned cienvaras and joegl and unassigned ahughes3 and joegl Sep 25, 2024
@ahughes3 ahughes3 temporarily deployed to Tugboat September 26, 2024 00:18 Destroyed
@cienvaras cienvaras requested a review from joegl September 26, 2024 00:26
@ahughes3 ahughes3 temporarily deployed to Tugboat September 26, 2024 15:04 Destroyed
@cienvaras cienvaras assigned joegl and unassigned cienvaras Sep 26, 2024
@ahughes3 ahughes3 temporarily deployed to Tugboat September 26, 2024 18:40 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat September 26, 2024 18:50 Destroyed
@joegl joegl merged commit 0d3d408 into 11.2.5-release Sep 26, 2024
15 of 16 checks passed
@joegl joegl deleted the SHS-5783--well-color-collections branch September 26, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants