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

3911 do not render empty lists on article page #4293

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

StephDriver
Copy link
Contributor

Updated django templates to check that there are galleys before creating download and checksum lists, across all three themes. Material already checked for galleys for download lists, but not for checksums.

For empty autogenerated tables of contents in OLH theme, updated the JS to remove the toc element if no headings were found, i.e. if list not populated.

Marked as draft as it is the end of the day, and I have yet to test this, so that must be done before sending for review.

@StephDriver StephDriver self-assigned this Jun 24, 2024
@StephDriver
Copy link
Contributor Author

StephDriver commented Jun 25, 2024

Download and Checksum were addressed on the template.

example shows Download, but Checksum done in same fashion.
Before:
image

<div class="section">
   <h3>Download</h3>
    <ul> 
    </ul>
</div>
image

After:

<div class="section">                             
    <h3>Download</h3>
     <p> Downloads are not available for this article.</p>
</div>

Autogenerated TOC addressed in the JS

This removes the "jump to" section completely.

@StephDriver
Copy link
Contributor Author

To Remove Empty Sections or Not to Remove...

For Checksum and Download, I have left the section there but added a statement that there are none available. This is because these are standard sections that a reader might 'look for' and so they should not be hidden but state clearly that they are empty.

But for the automated table of contents, I have removed the section entirely when it is empty. This is because the table of contents listing the sections does not make any sense in the context of an article without contents - and a reader would not look for it when there is no contents to navigate. To follow the pattern above with downloads and checksums would add to user confusion not prevent it.

@StephDriver
Copy link
Contributor Author

Closes #3911

@StephDriver StephDriver marked this pull request as ready for review June 25, 2024 11:17
@StephDriver StephDriver requested a review from joemull June 25, 2024 11:17
@StephDriver StephDriver assigned joemull and unassigned StephDriver Jun 25, 2024
Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

Nice one! I like the design decision to consistently display sections that are standard and often expected, like downloads and checksums.

I noticed a few things that we probably want to fix up for consistency between themes and screen sizes.

src/themes/OLH/templates/journal/article.html Outdated Show resolved Hide resolved
src/themes/clean/templates/journal/article.html Outdated Show resolved Hide resolved
src/themes/clean/templates/journal/article.html Outdated Show resolved Hide resolved
src/themes/material/templates/journal/article.html Outdated Show resolved Hide resolved
@joemull joemull assigned StephDriver and unassigned joemull Jun 26, 2024
@StephDriver
Copy link
Contributor Author

StephDriver commented Jun 26, 2024

@joemull please note response #4293 (comment)

for testing, I have tried all combinations of:

  • each of the 3 themes
  • wide or narrow windows
  • articles with or without galleys
    Anything I've missed?
Theme Galley Window Test Downloads Test File Checksums Test Table of Contents
OLH No Wide
OLH No Narrow
Material No Wide none
Material No Narrow none
Clean No Wide
Clean No Narrow
OLH Yes Wide
OLH Yes Narrow
Material Yes Wide
Material Yes Narrow none
Clean Yes Wide
Clean Yes Narrow

Following this, it seems both Clean and Material use the same toc.js. So now updating and rechecking that.

@StephDriver
Copy link
Contributor Author

Summary of Changes:

  • I have made the changes requested.
  • I retested the new functionality (as described in previous comment) and updated where needed.
  • I found that the Material and Clean themes use the same toc.js asset, which is in the Material directory, this seems to be a carbon copy of the toc.js in the OLH assets. I updated that toc.js and added the 'toc-section' id to the article.html files so that the toc-section can be hidden when no headings have been found while generating the toc.
  • The material theme tested fine, because it has other functionality which suppresses the TOC for short articles, such as the test example. The TOC card is not visible until one scrolls further down the page, and the page is too short to reach that point which is why it appeared to work correctly before the material toc.js update. I decided that for consistency, I would update the material article.html to be consistent with the other two themes, despite this... which means if later updates are made to the material theme which change when the TOC card is visible, such that it is visible on shorter pages, the purpose of this issue PR, to hide the TOC when there is no content for it, will then function consistently with the other two themes. But I am unable to directly test it working - I can note that it is exactly the same as in the Clean theme and that does work.

@StephDriver StephDriver requested a review from joemull June 26, 2024 14:37
@StephDriver StephDriver assigned joemull and unassigned StephDriver Jun 26, 2024
Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

Amazingly thorough! I just noticed one proofreader note that got left out of one of the galley checks.

src/themes/clean/templates/journal/article.html Outdated Show resolved Hide resolved
@joemull joemull assigned StephDriver and unassigned joemull Jun 27, 2024
@StephDriver StephDriver requested a review from joemull June 27, 2024 15:13
@StephDriver StephDriver assigned joemull and unassigned StephDriver Jun 27, 2024
@joemull joemull assigned mauromsl and unassigned joemull Jul 3, 2024
@joemull joemull requested a review from mauromsl July 3, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants