Skip to content

3911 do not render empty lists on article page#4293

Merged
StephDriver merged 10 commits into
masterfrom
3911-do_not_render_empty_lists_on_article_page
Aug 12, 2024
Merged

3911 do not render empty lists on article page#4293
StephDriver merged 10 commits into
masterfrom
3911-do_not_render_empty_lists_on_article_page

Conversation

@StephDriver

Copy link
Copy Markdown
Member

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

StephDriver commented Jun 25, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member 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
Copy Markdown
Member 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

@joemull joemull left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/themes/OLH/templates/journal/article.html Outdated
Comment thread src/themes/clean/templates/journal/article.html
Comment thread src/themes/clean/templates/journal/article.html Outdated
Comment thread src/themes/clean/templates/journal/article.html Outdated
Comment thread src/themes/clean/templates/journal/article.html
Comment thread src/themes/material/templates/journal/article.html Outdated
Comment thread src/themes/material/templates/journal/article.html
@joemull joemull assigned StephDriver and unassigned joemull Jun 26, 2024
@StephDriver

StephDriver commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Member 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

@joemull joemull left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/themes/clean/templates/journal/article.html Outdated
@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
@joemull joemull requested review from ajrbyers and removed request for mauromsl July 18, 2024 05:49
@joemull joemull assigned ajrbyers and unassigned mauromsl Jul 18, 2024

@ajrbyers ajrbyers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this @StephDriver this looks awesome. However, there is a conflict being flagged. Looks like an easy one though. If you need assistance with it let me know.

@StephDriver StephDriver force-pushed the 3911-do_not_render_empty_lists_on_article_page branch from b74686c to 29b4f17 Compare August 12, 2024 10:16
@StephDriver StephDriver merged commit 73de0b5 into master Aug 12, 2024
@StephDriver StephDriver deleted the 3911-do_not_render_empty_lists_on_article_page branch August 12, 2024 10:40
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.

4 participants