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

Add 'Back-to-top' button for partners list page #410

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Add 'Back-to-top' button for partners list page #410

merged 1 commit into from
Jul 28, 2020

Conversation

lalit97
Copy link
Contributor

@lalit97 lalit97 commented Feb 22, 2020

The partners page is quite long now. A 'back to top' button was needed.

Bug: T170113

@Samwalton9
Copy link
Member

Could you add a screenshot of what this button looks like, please? :)

@lalit97
Copy link
Contributor Author

lalit97 commented Feb 24, 2020

@Samwalton9 Sure adding a gif file showing the working of the button :)

back-to-top2

@lalit97
Copy link
Contributor Author

lalit97 commented Mar 4, 2020

Hello @Samwalton9 I thought you just missed this one, please have a look whenever you are free :)

@Samwalton9
Copy link
Member

Hey lalit - thanks for the reminder. I'm actually going to wait on reviewing this. We've got a lot of moving pieces being merged in to the tool this month and it might just reduce complications to hold off on merging this PR until then. Sorry about that - we'll get to it in a few weeks :)

@lalit97
Copy link
Contributor Author

lalit97 commented Mar 5, 2020

Okay, no problem :)

@jsnshrmn
Copy link
Member

I rebased this so I can check it against our current code base and review.

Copy link
Member

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

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

This occludes the footer navigation in mobile views. That needs to be resolved
Screenshot_2020-06-11 The Wikipedia Library Card Platform

When I load the page without JavaScript, this feature disappears. Please use JavaScript for progressive enhancement where possible. Since this is just navigation between page anchors, it should work without JavaScript.

TWLight/static/css/local.css Outdated Show resolved Hide resolved
@jsnshrmn
Copy link
Member

Also, we've moved to an anonymizing reverse proxy for external assets, so it would be great if you updated the URLs to match what is in #447. Since you had no way of knowing that this change was about to happen, I'd be happy to make that update on your behalf if you don't want to.

@lalit97
Copy link
Contributor Author

lalit97 commented Jul 27, 2020

This occludes the footer navigation in mobile views. That needs to be resolved

Fixed it, now working well in mobile view.

When I load the page without JavaScript, this feature disappears. Please use JavaScript for progressive enhancement where possible. Since this is just navigation between page anchors, it should work without JavaScript.

removed the javascript part.

@lalit97
Copy link
Contributor Author

lalit97 commented Jul 27, 2020

Also, we've moved to an anonymizing reverse proxy for external assets, so it would be great if you updated the URLs to match what is in #447. Since you had no way of knowing that this change was about to happen, I'd be happy to make that update on your behalf if you don't want to.

I have removed the urls, since they were already present in base.html. Thank you for such a detailed review. I have fixed all the points you have mentioned :)

@lalit97 lalit97 requested a review from jsnshrmn July 27, 2020 14:36
Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

This looks great and will be very useful once it's merged! I just want to request a small change before merging this.

TWLight/resources/templates/resources/partner_filter.html Outdated Show resolved Hide resolved
TWLight/static/css/local.css Outdated Show resolved Hide resolved
The partners page is quite long now. A 'back to top' button was needed.

Bug: T170113
@lalit97 lalit97 requested a review from suecarmol July 28, 2020 09:54
@jsnshrmn jsnshrmn merged commit b28fe8c into WikipediaLibrary:master Jul 28, 2020
@lalit97 lalit97 deleted the T170113 branch July 29, 2020 04:56
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

4 participants