Closed DavidFluck closed 4 years ago
Thanks!
A couple things:
I think there are no other footers to worry about as a practical matter, so I think the class is unnecessary here
You discussed this, but the desktop styles are just additive to the normal "screen" styles, so there's no need to duplicate this style in both places
On these themes we check in the compiled CSS, not just the Sass source, when we make changes
I do think we'd prefer the class on the footer markup removed and the style to just target footer
.
That should be the last thing, though, otherwise this looks good to go to me.
Apologies, I missed the e-mail. Thank you!
This fix addresses https://github.com/omeka/omeka-s/issues/1576.
Before:
After:
I've verified that this works for float left, center, and float right.
Summary:
Add clear: both to
<footer>
so previously floated elements, such as images, appear above the footer.Add
.page-footer
class to footer avoid polluting the styles of other footer elements.I wasn't sure if I should add this as a property of all footers or just this specific one, but to err on the side of caution to avoid affecting every footer, I gave this theme's footer a specific class for styling.
Additionally, I've updated both
_desktop.scss
and_screen.scss
. It looks like just adding this to_screen.scss
would've sufficed, but I wanted the fix to exist in both in case there was ever a chance that a media query would cause someone to receive a stylesheet that didn't have this fix.I'm happy to make any necessary fixes to bring this in line with project standards or to correct any misunderstandings on my part.