-
Notifications
You must be signed in to change notification settings - Fork 2
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
Wri 489 social links #356
base: main
Are you sure you want to change the base?
Wri 489 social links #356
Conversation
@mariacha This all installs perfectly on my local, but it seems to be having Circle issues - not sure how much of that is expected when there's Flagship and wri-sites things to be updated + 6 modules, four content types, etc. |
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.
Ok @komejo this is really impressive! Sorry for the huge lift here.
I fixed the things that were causing the build to fail and added a lot of comments that are largely about the same things.
@@ -1,3 +1,5 @@ | |||
uuid: 83bf409b-e85b-4756-bd54-7b2f9ae15d88 |
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.
Take off the uuid here please!
@@ -0,0 +1,135 @@ | |||
langcode: en |
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.
It looks like this file is causing the problems with install. We are only using it on Flagship, which is why. You can just delete this and the install function that pulls it in. (I've done this)
\Drupal::service('distro_helper.updates')->installConfig('core.entity_view_display.node.person.person_author_listing', 'wri_person'); | ||
\Drupal::service('distro_helper.updates')->installConfig('core.entity_view_display.node.person.search_results', 'wri_person'); | ||
\Drupal::service('distro_helper.updates')->installConfig('core.entity_view_display.node.person.small_image_teaser', 'wri_person'); | ||
\Drupal::service('distro_helper.updates')->installConfig('core.entity_view_display.node.person.teaser', 'wri_person'); |
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.
Since you're doing a more targetted update to the two display modes that matter below, you can skip everything but \Drupal::service('distro_helper.updates')->installConfig('field.field.node.person.field_paragraphs', 'wri_person');
here.
@@ -26,6 +29,7 @@ dependencies: | |||
- field.field.node.publication.field_publication_series | |||
- field.field.node.publication.field_region | |||
- field.field.node.publication.field_related_resources | |||
- field.field.node.publication.field_share_with_io |
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.
Since field_share_with_io
is only in the Flagship site, I took this off here.
@@ -0,0 +1,207 @@ | |||
langcode: en |
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 actually lives in web/profiles/contrib/wri_sites/modules/wri_superfeatured/config/install/core.entity_view_display.node.project_detail.superfeatured.yml
already -- I moved it.
* Add field_paragraphs and update all view modes. | ||
*/ | ||
function wri_program_update_10401() { | ||
\Drupal::service('distro_helper.updates')->installConfig('field.field.node.program_center.field_paragraphs', 'wri_program'); |
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.
And let's just leave the new field in here too.
closed_mode_threshold: 0 | ||
add_mode: button | ||
form_display_mode: default | ||
default_paragraph_type: social_media_sites |
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.
Can you change these to default_paragraph_type: _none
? Otherwise it's not clear that the social site isn't required (since fields within it are).
link: '' | ||
third_party_settings: { } | ||
weight: 9 | ||
region: share |
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 wasn't very clear with my comment, but for Projects, we want the share links to appear at the bottom of the main_content
region, under the body text. It can come out of the header and share sections entirely.
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.
OK, I think I have this right, but it was behaving oddly trying to export / save the updated configs.
weight: 3 | ||
additional: { } | ||
- | ||
uuid: c096c0b5-cccf-4994-94aa-99a39d69aec7 |
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.
Since this is being pulled in on the main_content
view mode, I don't think we need it here.
region: sidebar | ||
field_paragraphs: |
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.
And this can come out of the share region on the hero on a subpage too.
@mariacha As I was doing the PR feedback I realized that at some point while adding the field, configs, and update hooks for the person, program, and project content types, I became confused and also added it to publications, which... why would a publication need it's own social site?? Unless there was something somewhere that asked for it and now I can't find it? |
@komejo No you're right -- publications don't need it! I missed that while going through this as well, but yeah, it's just Person, Program, Project and PMicrosite. :) |
@mariacha OK, removing it was pretty easy - there shouldn't be any changes compared to |
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.
Ok, great -- this looks like it's in a pretty good place to me! I'll hold on merging until WRI can take a look, and either implement the feedback or pass back to you if I have any! I'm going to push up a couple of small changes first though.
What issue(s) does this solve?
Too limited social media links, all displayed in different ways. Affects programs, projects, microsites, and profiles.
https://github.com/wri/WRIN/issues/489
What is the new behavior?
Profile requirements:
Site-level pull requests for testing. Only merge when these PRs are approved:
Create or update any site-level pull requests following the documentation
Checked on develop (TA to do)