-
Notifications
You must be signed in to change notification settings - Fork 3
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-5070: Implementation: Switch H2 headings in collection components to H3 in some cases (Back End) #1405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hectorlj Looks good, just a couple of things:
- Please check CodeClimate errors
- "Title" field is correctly being conditionally set as required, but it also should be hidden when not required
- We'll probably need an update hook to set a value for existing collections. Let's confirm with Amit on today's check-in.
Thanks @cienvaras! I also realized that I need to update the codeception collection test, and the form display conditional that hides the title needs an update hook. |
…i into shs-5070-switch-headings
@hectorlj Works great, thanks! Not merging yet because I'll be adding the FE work on this same branch, but we can consider the BE part as complete. |
@mndonx Can you please take a look at the FE part of this PR? I added instructions in the description. Files to include in FE review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! The only comment I have is maybe leave a comment or two to explain what the functions are doing. Otherwise, I have tested it locally and it works as expected.
… shs-5070-switch-headings
READY FOR REVIEW
Summary
Title
andTitle Settings
fields to thehs_collection
paragraph typeSteps to Test
Backend
Title
andTitle Settings
fields are in the componentTitle
field is set to required when it is visibleTitle
field is hidden and not required when `Title Settings is set to the following values:I've already created a Heading 2 above this Collection
I do not want a heading for this Collection
Frontend
<h2>
<h2>
, but visually hidden<h3>
. Otherwise, they will be rendered as an<h2>
<h2>
and<h3>
(*) The following components don't have a header, you can skip them when testing: Text Area, Accordion, Banner Image and Testimonial.
PR Checklist