-
Notifications
You must be signed in to change notification settings - Fork 70
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
VACMS-10626: Drupalize new homepage components #11016
VACMS-10626: Drupalize new homepage components #11016
Conversation
I noticed you encountered a test failure on Cypress. It's failing as a result of a 502 error, and I'm paying attention to these (see #10614 ), but haven't gotten much traction yet 😭 |
Yeah I'm not sure what to make of that. I ran at least 3 different builds yesterday, each had unique test failures unrelated to my changes either in behat, cypress, or both, and this is just the latest of them. |
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.
Thank you for the awesome QA steps. Made it a breeze.
A handful of questions.
I don't see perms on this. Are those still being determined and coming later?
config/sync/core.entity_form_display.block_content.benefit_promo.default.yml
Show resolved
Hide resolved
| VA Wichita health care | va-wichita-health-care | VISN 15 \| va.gov/wichita-health-care | | ||
| VA Wilkes-Barre health care | va-wilkes-barre-health-care | VISN 4 \| va.gov/wilkes-barre-health-care | | ||
| VA Wilmington health care | va-wilmington-health-care | VISN 4 \| va.gov/wilmington-health-care | | ||
Scenario: Menus |
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.
If you have any behat auto formatter running on your IDE we want to shut that down. It should take raw from the spectool Otherwise it makes it look like all these lines changed.
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.
I don't have anything auto-formatting behat tests, these are copy/pasted from the spec tool directly. It is possible I copied some surrounding text or otherwise made a boo boo here. I'll correct the formatting so changes are easier to detect (I have a change to make in the content model anyway).
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.
The other possibility is that menu tests have never been exported from airtable and so the formatting is just a result of the switch from google sheet to airtable... but i am pretty sure we did an export of all of them so that the switch only happened once in one PR that was just for that purpose.
counter_position: after | ||
js_prevent_submit: true | ||
count_html_characters: true | ||
textcount_status_message: 'Maxlength: <span class="maxlength_count">@maxlength</span><br />Used: <span class="current_count">@current_length</span><br />Remaining: <span class="remaining_count">@remaining_count</span>' |
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.
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.
I was unaware there was a pattern, that is good to know. The character limits have not come back from Michelle yet, so I added some extremely generous limits for the time being to satisfy the AC as well as to help communicate that the limits have not been properly tuned for the context.
@@ -0,0 +1,79 @@ | |||
uuid: dc8c65f9-4223-4e21-b201-a979b27118ae |
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.
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.
- I don't know, who decides that?
- Correct, I don't have guidance text provided, so it is not included in the PR
- I don't know, who decides that?
- I'll fix the revision settings so they match
- I don't know, who decides that?
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.
I think most of these would come from UX. @BlakeOrgan as he comes up to speed or @EWashb . For some of these they should stick with existing patterns unless there is reason to deviate.
I understand there is some need to get the fields out there so that the FE work can get started. I think it is acceptable to kick the UX part down the road in the form of another ticket if decisions on them are not ready to be finalized.
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.
I agree with @swirtSJW. If you need to get the build out right now for the next tech step that's fine as long as it's not going out to editors like this. I would agree that we need some UX input on the writing here and should try not to deviate from existing patterns too much.
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.
If you need to get the build out right now for the next tech step that's fine as long as it's not going out to editors like this.
I'm not sure how I can get this out without affecting the editors. Once this is merged, the new block types will be visible in the CMS.
UX Design input is in the ACs for this issue, but it wasn't clearly specified how that would be incorporated into the flow of this ticket. So, in order to get the UX design feedback kicked off as early as possible, I PR'd what I could based on the information I had available.
For some of these they should stick with existing patterns unless there is reason to deviate.
I appreciate this is the goal, but it is really difficult to see patterns when I touch so little of the code, and deviations exist. Please do what you can do call out specific deviations from patterns that you see, as it will help me learn those over time.
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.
@dsasser is there something I can do for you now to help get this over the finish line? Are you on a timeline to have this out before the end of the sprint?
@@ -0,0 +1,79 @@ | |||
uuid: c9e89373-ad1e-4d92-98c5-5ef003aaa3fc |
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.
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.
Similar feedback to guidance text and form elements missing like on the Benefit promo.
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.
That is a good question, honestly. I gave it a good amount of thought while developing, and I ended up keeping the blocks separate because of my concern that keeping the field settings in lock-step might prevent us from having unique treatments/validations for the different situations, if that makes sense. I was concerned that the somewhat fluid details were lending themselves to having the most flexibility on the CMS side, erroring on the side of caution if you will.
Also, it didn't really occur to me that the content models could be identical (it took several iterations before I was satisfied) until they were already established. Looking back, yeah it definitely makes sense to combine them, providing we are ok with that from a UX perspective.
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.
Makes sense.. and yes if there is a strong likelyhood of fluid changes to one but not the other down the road, then I am happy to support that unknown.
dependencies: | ||
config: | ||
- block_content.type.benefit_promo | ||
- field.field.block_content.benefit_promo.field_owner |
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.
I think we want to go with field_administration rather than field_owner
According to the spec tool we have 51 instances of field_administration and 9 instance of field_owner.
field_owner was. I wish I could recall the rationale for moving away from field_owner, I don't recall.
It looks like in most cases the field_owner is applied to non-node entities (except for one outlier which I know was a mistake that snuck through) I could have my arm twisted to stick to that pattern since I don't recall the original rationale.
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.
I was unaware of this pattern, thank you for pointing it out. I'll move to field_administration then.
@@ -0,0 +1,8 @@ | |||
uuid: 9a594b0d-1ab2-44eb-8b0b-92e92dba96c5 |
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.
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.
Thanks for pointing this out. I don't have a great grasp on permissions/workbench access yet. As a result, permissions have not been setup. We have a meeting tomorrow to discuss this where I hope to confirm some of my assumptions about how the sections + workbench access. I should have been more clear in the QA steps that permissions were not yet ready.
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.
I'm also fine with punting perms down the road with a followup ticket if we are just trying to get the content model out there so FE work can get unblocked.
Closing to move to integration branch. |
Description
Closes #10626
New Block Types:
Benefit Promo
News Promo
New entityqueues:
Home page news spotlight
Home page hero
New Menus:
Other Search Tools
Popular on VA.gov
QA steps
Add one or more links to the popular-on-va-gov menu: https://pr11016-b9g0h6rk9avw9mnrfirx6ypjsgssems7.ci.cms.va.gov/admin/structure/menu/manage/popular-on-va-gov
Run GraphQL queries:
Validate GraphQL queries provide expected output:
Definition of Done
Select Team for PR review
Platform CMS Team
Sitewide program
⭐️ Sitewide CMS
⭐️ Public websites
⭐️ Facilities
⭐️ User support
Is this PR blocked by another PR?
DO NOT MERGE
Does this PR need review from a Product Owner
Needs PO review
CMS user-facing annoucement
Is an announcement needed to let editors know of this change?