-
Notifications
You must be signed in to change notification settings - Fork 5
BREAKING: Add Footer implementation #208
Conversation
@joewalsh What's the main reason to break backwards compatibility for the footer transform? |
@berndsi it's not worth investing effort in making the footer transform backwards compatible when it'd only be used for a short while before the iOS app upgrades to mobile-html. It's easier to have a clean break and implement it solely in the best way for mobile-html. |
src/transform/FooterContainer.css
Outdated
padding-top: 35px; | ||
font-size: 0.8em; | ||
line-height: 0.8em; | ||
color: #72777D; | ||
} | ||
|
||
.pagelib_theme_sepia .pagelib_footer_container_heading { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to keep those rules empty? Why not delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. There had been several empty rules in this CSS file before. @joewalsh Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these empty rules can be removed. Should remove the classes in the HTML as well. They might need to be re-added depending on design tweaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit inline that I don't think is a big deal. Overall, LGTM.
I can't officially approve because it was originally my PR, but looks good to me 👍 |
LGTM to me too. |
@joewalsh Thank you for fixing the docs. |
rebased |
Parsoid output has the dir=ltr/rtl on the body instead of the root html element.
The update script requires 'request' which should be installed locally. That way this doesn't need to be installed globally anymore.
It's a bit too much code for the high level PageMods to live in, and seems that it could be a separate and optional feature.
Had changed the name, which should be reflected in the documentation.
Also removed the classes in the HTML. We can add those back if we need them in the future. I left the ids in the HTML, though.
rebased again. There were some conflicts that needed to be resolved first. |
https://phabricator.wikimedia.org/T217352
To test locally with a local instance of restbase running, use:
And then to add a bookmark icon and text to a single Read more item:
Other notes:
This isn't intended to keep working as it did when bundled with the iOS app. The iOS app can stay on an older version of page library and the newer changes should be solely for mobile-html.