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

SDSS-594: Added media contacts entity and field #217

Merged
merged 26 commits into from
Sep 25, 2023

Conversation

joegl
Copy link
Collaborator

@joegl joegl commented Aug 24, 2023

READY FOR REVIEW

Summary

  • Created an sdss_entities module for custom SDSS entities.
  • Created a new Media Contact entity type.
  • Created a Media Contact UI Pattern
  • Created a Media Contact field on News.

Review By (Date)

  • 8/29

Review Tasks

Setup tasks and/or behavior to test

JEN @jenbreese
Review the Media Contacts field (you will be adjusting the display/template/styles):

  1. Check out this branch
  2. Refresh stack drush ci, drush cr
  3. Navigate to a News node
  4. Add Media Contacts
  5. Verify you can see the Media Contacts on the News page

MIKE @pookmish

  1. Review code, specifically sdss_entities module. I'm pretty sure there's some stuff I may not need in there.

Front End Validation

  • Is the markup using the appropriate semantic tags and passes HTML validation?
  • Cross-browser testing has been performed?
  • Automated accessibility scans performed?
  • Manual accessibility tests performed?
  • Design is approved by @ user?

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

https://stanfordits.atlassian.net/browse/SDSS-594

Resources

@jenbreese
Copy link
Contributor

@joegl Its looks ok for me to style.

Copy link
Contributor

@pookmish pookmish left a comment

Choose a reason for hiding this comment

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

Questions and suggestions

@pookmish
Copy link
Contributor

How are these handled/managed different than nodes?

@joegl
Copy link
Collaborator Author

joegl commented Aug 25, 2023

How are these handled/managed different than nodes?

@pookmish They wanted to create a content reference, and we/I didn't want them to be nodes or content types, because they're only meant to be re-usable inline content for the existing content types, and not managed or used independently. After starting the ECK work for H&S, the idea to make a custom entity clicked and I imagine we might make a few others in the future in the module.

@pookmish
Copy link
Contributor

only meant to be re-usable inline content for the existing content types

Why would the need to have labels to "organize the entities" then?

@joegl
Copy link
Collaborator Author

joegl commented Aug 25, 2023

Why would the need to have labels to "organize the entities" then?

They wanted to be able to re-use a media contact multiple times. But they also want to retain the ability customize a media contact for a specific page (i.e., they want to be able to create a media contact with the same name, but different values for the other fields and be able to distinguish between them from a referential standpoint). So every once in a while there will be a person that might have 2-3 different "Media Contacts" for them depending on what contact info they want to use for that article.

@jenbreese
Copy link
Contributor

jenbreese commented Aug 29, 2023

@joegl After starting to theme it, I noticed a couple of things.

  1. It would be nice if the output order matched the design order which is name, school, phone and email. - Done in theming branch.
  2. The email needs to be wrapped in a mailto link. - Done in theming branch.
  3. The names need to be an h3. I confirmed it with @cjwest. - Done in theming branch.
  4. Also, we need some sort of help text for the label so the author knows it won't be displayed and is only for the authors.
  5. I noticed I could add more than one Label with the same name. Do we want to not allow that?

@joegl
Copy link
Collaborator Author

joegl commented Sep 13, 2023

Thanks Jen and Mike for the review.

@pookmish I addressed all the feedback and removed the cruft. I did leave the Label field because it's used as the lookup value for entity reference autocompletes AND the displayed value when a referenced entity is collapsed.

@jenbreese Regarding your feedback #4 and #5 -- the "label" field is built into Drupal's entity system and I'm not exactly sure how to add help text to it or force the value to be unique. These are things I feel we can progressively enhance on as needed.

@joegl joegl requested a review from pookmish September 13, 2023 19:30
* SDSS-885: NEWS | Style Media Contacts display.
Copy link
Contributor

@jenbreese jenbreese left a comment

Choose a reason for hiding this comment

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

The styling looks right and works correctly. I created new media contacts and then used them on other news pages. I also used a mix of new ones and existing ones. They worked as I would have expected.

@joegl
Copy link
Collaborator Author

joegl commented Sep 22, 2023

@pookmish Can you give this a second pass? Thanks

@joegl joegl merged commit d0dc2c2 into 2.x Sep 25, 2023
4 checks passed
@joegl joegl deleted the SDSS-594--media-contacts-entity-component branch September 25, 2023 20:28
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.

3 participants