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-1299: Add Explore More options and display to News #466

Merged
merged 10 commits into from
Aug 28, 2024

Conversation

buttonwillowsix
Copy link
Contributor

@buttonwillowsix buttonwillowsix commented Jul 18, 2024

READY FOR REVIEW

Summary

  • Created curated and random views for Explore More so that content authors can display more relevant content and so that more content is surfaced to readers of news
  • Created new field to allow author to choose which view to display, with random view as default

Review By (Date)

  • In time to release pre-Aug 31

Criticality

-HIGH

Review Tasks

Setup tasks and/or behavior to test

Test the random default view

  1. Check out this branch
  2. Create a bunch of news. Tag them with a variety of News Types
  3. Verify that random/different news items show up in the Explore More section for each news item (not the same ones on every page)

Test the contextual filter on the Random display

  1. Edit one of the news items.
  2. Go to the Options for Explore More field
  3. Enter an argument for one of the terms you used when you tagged news
  4. Verify that the randomized view only contains news tagged with that term

Test curated displays

  1. Edit one of the news items
  2. Under the Related Content field, select three other news items
  3. Go to the Options for Explore More field, and select the Display "News that is selected as related on this article"
  4. Verify that Explore More shows your selected news items
  5. Now edit one of the news items that you selected as Related Content
  6. Go to the Options for Explore More field, and select the Display "News that has selected this article as related"
  7. Verify that Explire More shows only the news item from Step 1.

Front End Validation

No new front-end code

Backend / Functional Validation

Code

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

General

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

Affected Projects or Products

  • SDSS News
  • News throughout the SDSS stack

Associated Issues and/or People

- SDSS-1299

Resources

@mdyoung3
Copy link
Contributor

@buttonwillowsix it looks like "Explore More" is showing when there's no results.

Screenshot 2024-07-26 at 6 31 17 PM

Copy link
Collaborator

@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.

@buttonwillowsix This is really neat! I tested everything and only had 1 spot where it wasn't working correctly. I also had a few questions comments.

  • Test the random default view. This worked perfectly.
  • Test the contextual filter on the Random display. This did not work. It was still completely random. I created 5 articles, 3 with announcement as the News Type and 2 with Blog as the News type. I edited an article and entered "Blog" in the arguments for the Random display and it still was completely random (it showed both Announcement articles and Blog articles).
  • Test curated display: "News that is selected as related on this article". This worked, however I had to make sure to clear the arguments. I also think this should be re-labeled. Maybe "Use related content selections"? I find the names confusing.
  • Test curated display: "News that has selected this article as related". This also works, but again the labeling could maybe be more clear?

A few other questions/concerns:

  • There is inconsistent labeling. One display is prefaced with "Explore More -" while the other two are prefaced with "Explore More News -".
  • The arguments field still works for the two Curated displays. Is this intentional?
  • The Explore option allows you to set the "View" to "None" but it still displays the "Explore More" header (with nothing underneath it). Is this something we want to prevent?
  • If there are no relevant results for the Explore More view, it displays the "Explore More" header with nothing underneath it. What behavior should happen there?

@buttonwillowsix
Copy link
Contributor Author

These are all great comments! And I like your improved suggest for labeling the curated view. I struggled a ton with how to describe these and was hoping to be struck with inspiration that never came.

I will fix this stuff up in the branch (I can make that my next priority today) and then pass it back to you.
The taxonomy filtering on the random view is not critical for launch, so if I can't determine what is wrong with that, I will remove it for now. I think I could also remove it from the curated views (although I can see an argument for using it on the indirectly curated view.)

I will see what I can do about the header. I like the option to have "none" there because there may be cases where that would be appropriate.

@joegl
Copy link
Collaborator

joegl commented Aug 23, 2024

Sounds good! I think removing the filtering makes sense for now as an MVP and could be added later. Contextual filters can be time consuming.

@buttonwillowsix
Copy link
Contributor Author

buttonwillowsix commented Aug 23, 2024

The contextual filter on the random display was not set up (which is why it didn't work.) The only filter I have on there is the one to filter out the current piece of news! I will not be making changes there. The same is true on the other two displays. It is working as designed in that respect!

@joegl
Copy link
Collaborator

joegl commented Aug 26, 2024

@buttonwillowsix Is this ready for review again?

@joegl joegl self-requested a review August 27, 2024 15:51
Copy link
Collaborator

@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.

❌ I saw the <h2> is hard-coded into the view in a custom HTML field now to hide it when there's no content. Is there a way to hide the title if the view block is empty using Layout Builder? It'd be nice to not have this <h2> hard-coded in a custom text field in the view header.

❓ The argument is not working for any of the views now. I tested multiple scenarios with different News Type terms, and they are always empty. I assume this is fine for now?

✔️ Updated labels look good.

@buttonwillowsix
Copy link
Contributor Author

@joegl I will see if Layout Builder will take care of it now that I added some "hide this view if empty" to the settings. I think my problem was that if I show the view header, I don't have a way to style it without adding a template to get an H2 and a class on view header. I don't think we want people changing the title on individual pages, but I agree it is more elegant to use Layout Builder. I will look this AM.

There is no contextual filter available for the user to interact with, so that is totally fine. I wish we could hide that field if the only contextual filter is the one that filters out the current node, but I don't think there is. It probably would be easy to add a topical contextual filter on that, though. If you'd like, I could add that in (especially since this seems unlikely to make this release at this point.. hoping we can release it pretty soon, though!

@joegl
Copy link
Collaborator

joegl commented Aug 27, 2024

@joegl I will see if Layout Builder will take care of it now that I added some "hide this view if empty" to the settings. I think my problem was that if I show the view header, I don't have a way to style it without adding a template to get an H2 and a class on view header. I don't think we want people changing the title on individual pages, but I agree it is more elegant to use Layout Builder. I will look this AM.

There is no contextual filter available for the user to interact with, so that is totally fine. I wish we could hide that field if the only contextual filter is the one that filters out the current node, but I don't think there is. It probably would be easy to add a topical contextual filter on that, though. If you'd like, I could add that in (especially since this seems unlikely to make this release at this point.. hoping we can release it pretty soon, though!

Sounds great Dena. I'd be happy to get this out today as-is if we can figure out the title issue though that'd be great.

@buttonwillowsix
Copy link
Contributor Author

@joegl I think if we want to use the view header, I will need to add a twig template to wrap the view header in an h2 and the block-title class. I haven't done that in ages, and probably would not have time to add that until maybe Friday at the earliest. Let me know if that sounds like a good approach.

@joegl
Copy link
Collaborator

joegl commented Aug 27, 2024

@buttonwillowsix It's too bad the viewfield display block doesn't have a way to hide the entire block (including the title) if the view contents are empty. That would seem most straightforward.

@buttonwillowsix
Copy link
Contributor Author

@buttonwillowsix It's too bad the viewfield display block doesn't have a way to hide the entire block (including the title) if the view contents are empty. That would seem most straightforward.

It does have a way to hide the entire block if it is empty (including title,) I just don't have a way to style the title without either a) a FE dev to add the styling in the theme (I don't think we want this because we also want it to be an H2) or b) adding a template to wrap the title in existing styling.

It sounds like you'd prefer that we take the approach of using the view title, so I can do that (again, probably not until Friday. I need a quiet moment to remember even how to turn on the theming suggestions so I can remember how to make the template!)

@joegl
Copy link
Collaborator

joegl commented Aug 27, 2024

@buttonwillowsix The block title should already be styled if it's using layout builder shouldn't it? I can definitely look at styles if needed.

@buttonwillowsix
Copy link
Contributor Author

buttonwillowsix commented Aug 28, 2024

@buttonwillowsix The block title should already be styled if it's using layout builder shouldn't it? I can definitely look at styles if needed.

Yes! That is the one that is styled. But the Layout Builder title appears even if the block is blank. So we have to use the view title. Layout builder can use the view title, but that is not styled. If this helps:

  • Title in layout builder: styled but won't disappear when blank
  • Title in view: will disappear but not styled
  • Dena's fix: title in view header (styling + disappears along with the view)

@joegl
Copy link
Collaborator

joegl commented Aug 28, 2024

@buttonwillowsix The block title should already be styled if it's using layout builder shouldn't it? I can definitely look at styles if needed.

Yes! That is the one that is styled. But the Layout Builder title appears even if the block is blank. So we have to use the view title. Layout builder can use the view title, but that is not styled. If this helps:

  • Title in layout builder: styled but won't disappear when blank
  • Title in view: will disappear but not styled
  • Dena's fix: title in view header (styling + disappears along with the view)

👍 I misunderstood when you said "It does have a way to hide the entire block if it is empty (including title)".

@buttonwillowsix
Copy link
Contributor Author

Make field required. Then put no results text in view. Go back to Layout Builder title.

Copy link
Collaborator

@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.

Thanks for all the work and collaboration on this Dena. I think it's going to be a huge improvement.

@joegl joegl changed the title SDSS-1299: Explore More Field SDSS-1299: Add Explore More options and display to News Aug 28, 2024
@joegl joegl merged commit f8fbd72 into 4.x Aug 28, 2024
5 checks passed
@joegl joegl deleted the SDSS-1299-explore-more-field branch August 28, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants