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.

Sign up for GitHub

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

Jump to bottom

Add tag template to News theme #139

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Add tag template to News theme #139

merged 2 commits into from
Sep 13, 2023

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Aug 31, 2023 •
edited by jazairi
Loading

This makes the tag template of the News theme into something usable. As a stretch goal, it also refactors the content-single.php partial into a more usable and sustainable condition in a separate commit.

These changes will result in a more usable display of content for visitors who click a link on the front page of DSpace@MIT.

Links to the review app and more context are in the linked ticket.

Ticket: https://mitlibraries.atlassian.net/browse/PW-55

Note: during evaluation of this change via ANDI, I realized that there are some color contrast problems in the News theme. They are not introduced with this change, but they exist in the tag template because of how the author line (and event information) are rendered. I've created https://mitlibraries.atlassian.net/browse/PW-56 in the backlog to tackle this issue among our other work.

Developer

Stylesheets

  • Any theme or plugin whose stylesheets have changed has had its version
    string incremented.

Secrets

  • All new secrets have been added to Pantheon tiers
  • Relevant secrets have been updated in Github Actions
  • All new secrets documented in README
  • No secrets are affected by this change

Documentation

  • Project documentation has been updated
  • No documentation changes are needed

Accessibility

Stakeholder approval

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies

NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

@matt-bernhardt
Adds tag template to News theme
1a08b23 
** Why are these changes being introduced:

* DSpace@MIT currently sends traffic to a page showing all news articles
  with a specific "dspacemit" tag - the only such example of a tag index
  on the site. Unfortunately, this tag index is not displaying correctly
  due to a bug in the parent theme's tag template.
* On the legacy site, this display functions, but is a poor fit for the
  need because the display isn't branded correctly (it is partially
  branded with the parent theme)

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/pw-55

** How does this address that need:

* This adds a tag template to the News theme. It repeats the structure
  of the legacy display, being a paginated list of complete articles,
  instead of the category index that shows an AJAX-paginated list of
  cards.
* There is a small change to content-single.php in the News theme, which
  we realized was previously unused. Now it is used by the tag template,
  but we need to change one class to control how the article metadata is
  displayed (the former class name results in an absolutely-positioned
  element on screen, which was useful in a card display but is a bug
  here).
* The news styles are updated to ensure that large images don't overflow
  their container, and to add some padding around each news article in
  the list.
* The parent styles are updated to ensure that pagination links are
  placed alongside each other.
* The news theme version is bumped to 0.3.0. This is a nice-to-have,
  not required by the style changes as the changed stylesheets are not
  compiled by SCSS.

** Document any side effects to this change:

* The page structure we're using for the tag index is a new visual
  approach, but the work to provide an AJAX-powerd paginated list of
  cards would have been higher, and isn't justified by how little use
  this template will likely see.
@matt-bernhardt
Stretch goal: refactoring content-single.php
2c71816 
** Why are these changes being introduced:

* As we are now using content-single.php (and discovering that it was
  unused before), we are discovering that there is a pretty significant
  amount of debt in the template - unused conditionals, references to
  fields that no longer exist, etc. That made working with this template
  a bit trickier than needed because it wasn't clear what was needed.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/pw-55

** How does this address that need:

* This refactors the template, removing unused variables, unused
  conditionals, etc. Full details are below.
* The mark_as_new field no longer exists, so that check is removed.
* The article category is not shown, and the old logic never actually
  displayed it - so we remove it entirely.
* There are no "features" or "updates" post types on the network, and
  the "exhibits" post type is not used on the News site - so those
  checks are also removed.
* The bespoke handling of event date and time information is removed in
  favor an existing partial that is used elsewhere in the theme. While
  we don't anticipate any event information to be displayed using this
  template (The DSpace@MIT tag has always been news articles written by
  our staf, and not event records imported from the central MIT
  calendar), this will protect us in the future if that changes.
* Whitespace is cleaned up, and references to $subtitle and $type
  variables that are never used are removed.

** Document any side effects to this change:

* None, unless I've missed a path somewhere in the theme where this
  partial is loaded somewhere other than tag.php. If that is the case,
  then there will likely be an impact in how content is rendered that
  I have not seen or considered.
@jazairi jazairi self-assigned this Sep 12, 2023
@jazairi
Copy link

jazairi commented Sep 12, 2023

The tag template seems to be working as expected on the news theme. I'm not sure how to verify the changes in content-single.php, but the refactored code seems to make sense. :shipit:

@matt-bernhardt
Copy link
Member Author

matt-bernhardt commented Sep 13, 2023 •
edited by jira bot
Loading

Oh, I don't know if I ever documented how to verify which templates are used in building a given HTML page. content-single.php is now used on the tag template. I've made ticket PW-59 to create this.

@matt-bernhardt matt-bernhardt merged commit 6b65d9e into master Sep 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jazairi jazairi jazairi approved these changes

@JPrevost JPrevost Awaiting requested review from JPrevost

Assignees

@jazairi jazairi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matt-bernhardt @jazairi