Wrapped each shared file post in an <article> tag and used <nav> for older / newer navigation controls #850
Conversation
… up an image post in a single <article> tag. Better use of semantic html and will lead to simpler styling down the road. Similarly, use a nav tag instead of a div to wrap bottom of page older / newer controls, and remove a couple of superfluous wrapper divs.
…hether they're first or last in the page. Useful for future navigation and styling.
| <div class="linear-navigation"> | ||
| <div class="older"> | ||
|
|
||
| <nav class="linear-navigation timeline-navigation" aria-label="Timeline"> |
There was a problem hiding this comment.
Is there a purpose for the timeline-navigation class? I don't see any reference to it here or over at mltshp-patterns.
There was a problem hiding this comment.
Feel that timeline is more descriptive than linear for what the navigation does. Hence the aria label. Wanted the classname to reflect that, but with a plan to introduce gradually (phasing out linear-navigation in patterns over time).
| <a class="older btn btn-secondary btn-shadow" href="{{ older_link }}">« Older</a> | ||
| {% else %} | ||
| {# Simplify container layout #} | ||
| <div></div> |
There was a problem hiding this comment.
I would expect simpler to mean fewer DOM elements. Also, this is a block element where the other is inline. But my preference would be to not emit anything if older_link or newer_link are "false". If both are false, then nav itself can be removed since there's no appropriate navigation (layout would still need to look right, with respect to padding at the bottom of the short page).
There was a problem hiding this comment.
Simpler was also referring to CSS complexity. Let me see if there's a way to do nicely with CSS alone ...
| {% else %} | ||
| {% if sharedfile.get_title() == "" %} | ||
| <h1 class="image-edit-title the-title the-title-blank">click here to edit title</h1> | ||
| <article id="shared-file-{{sharedfile.share_key}}" class="shared-file{{ " first-on-page" if sharedfile.first_on_page else "" }}{{ " last-on-page" if sharedfile.last_on_page else "" }}"> |
There was a problem hiding this comment.
I might be missing something, but I don't see what effect the id and first-on-page and last-on-page classes have. There's nothing in paging_keys.js that changes behavior based on the class names. I'd expect the #shared-file-[share_key] anchor to be used for the newer link so it scrolls down to that item on navigation to a newer post.
There was a problem hiding this comment.
This was the same as timeline-navigation - speculative class to transition to in patterns over time.
Unique id for each article would allow us to scroll to a particular post in a list or other context. Alternatively could use first-on-page or last-on-page in jquery selector to achieve a similar scroll. Either or both of these could be helpful in resolving #852.
Suspect we could probably use CSS last-of-type instead of first-on-page or last-on-page, though included them in this PR as setting up some per-article markup we could use to fix other problems.
|
Generally speaking, do I need to have a transition plan for introducing new dependencies between mltshp and mltshp-patterns? I didn't necessarily introduce a hard dependency between them that required a synced release. If I tidy up the first-on-page (probably going to discard for a CSS last-of-type approach anyway) and the timeline navigation section, are you generally supportive of the structure change? If so I'll press on and make the changes to other list generation pages. |
Changes that require CSS modifications - including any element hierarchy changes that could affect layout - should be done in the mltshp-patterns repo (amending the example page that demonstrates the changes). Once that is approved, it can be incorporated into this PR, along with the new minified stylesheet. |
|
Gotcha. Had it backwards. Will do. |
2 participants
Wrapped each pair of divs (header and image content) that currently make up an image post in a single
<article>tag. Better use of semantic html and will lead to simpler styling down the road.Similarly, used a nav tag instead of a div to wrap bottom of page older / newer controls, and removed a couple of superfluous wrapper divs. Both these will improve navigability for screen readers.
In both cases visual layout remains unchanged.
Looks like a lot though if you ignore whitespace the changes are really only:
<article><header>rather than a div to wrap the post header<nav>If these look ok, there are a couple more places to replicate the timeline changes before merging e.g. search results.
Resolves #848