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

Feature #9108 implemented. A scroll to top anchor tag added. #9111

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nishantjo-c
Copy link

@nishantjo-c nishantjo-c commented Oct 1, 2023

Fixes: #9108

Smooth scroll back to the top implemented for user convenience.

Copy link
Member

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

This is a good start but there are a few things missing. Please checkout the must-have list: #9108 (comment)

And please make sure to only commit the relevant changes and don't change any formatting:

  • Revert Gemfile.lock
  • Revert blanks in v2.html
  • Revert main.css
  • Revert formatting in docs.js

@nishantjo-c
Copy link
Author

I have added the screenshots in the previous commit.
The design is changes as required.

Copy link
Member

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Please place the "back-to-top" button on the right bottom corner and make it much smaller and also make the arrow centered

docs/Gemfile.lock Show resolved Hide resolved
docs/Gemfile.lock Show resolved Hide resolved
docs/_layouts/v2.html Outdated Show resolved Hide resolved
docs/_layouts/v2.html Show resolved Hide resolved
docs/docs/js/docs.js Show resolved Hide resolved
docs/docs/js/docs.js Show resolved Hide resolved
@nishantjo-c
Copy link
Author

Changes made. Made the button response with screen size.
Review the design and tell me if some more changes required.

Copy link
Member

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Please revert the not necessary changes like in the Gemfile and the comments in v2.html. I will not review your code again until this changes are reverted.

Also please update your branch with the newest main branch

Comment on lines 164 to 166
&:hover{
opacity: .5;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the classic css definition for now.

#backtotop:hover{
		opacity: .5;
	}

height: 26px;
right: .2em;
}
.backtotop{
Copy link
Member

Choose a reason for hiding this comment

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

Please rename the class or the id. They should not have the same name

<a class="ext-link github" href="http://github.com/Leaflet/Leaflet" title="View Source on GitHub"><img alt="View Source on GitHub" src="{{root}}docs/images/github-round.svg" width="46" /></a>
<a class="ext-link forum" href="https://stackoverflow.com/questions/tagged/leaflet" title="Ask for help on Stack Overflow"><img alt="Leaflet questions on Stack Overflow" src="{{root}}docs/images/forum-round.svg" width="46" /></a>
<div id="backtotop" class="ext-link"><div class="backtotop"></div></div>
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense, to have this not in this <nav> element, because it is displayed at the bottom

Copy link
Author

Choose a reason for hiding this comment

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

Yeah! Before it was just next to the nav elements. And then I re positioned it to the right bottom. I will re-position it to somewhere else.

@nishantjo-c
Copy link
Author

Hey i have made changes and requested a review.

@Falke-Design
Copy link
Member

Please revert the not necessary changes like in the Gemfile and the comments in v2.html. I will not review your code again until this changes are reverted.
Also please update your branch with the newest main branch

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.

scroll-up button
2 participants