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 "go back to top" arrow #547

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Add "go back to top" arrow #547

merged 3 commits into from
Oct 27, 2023

Conversation

VaibhavSharma24
Copy link
Contributor

Created the toggle button to move to top of the home screen issue #539

js/guides.js Outdated Show resolved Hide resolved
Copy link
Member

@tombruijn tombruijn left a comment

Choose a reason for hiding this comment

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

Please rename the arrow/button to a more descriptive name.
Use the scrollTo function.

Let me know if you can replace the image with something like the CSS arrow I linked (not exactly that one please). I can also pick that up for you.

Comment on lines +107 to +109



Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Unnecessary empty lines.

js/guides.js Outdated Show resolved Hide resolved
arrow.png Outdated
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 we can also use something made in pure CSS. No need for an image. Something like this would work: https://codepen.io/tombruijn/pen/ExGqVdx
Also easier to color on hover for example.

index.html Outdated
@@ -29,6 +29,8 @@ <h2>Guides in other languages</h2>
</div>
</div>

<button onclick="topFunction()" id="myBtn" title="Go to top"><img class="arrow-img" src="arrow.png" alt="" srcset=""></button>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<button onclick="topFunction()" id="myBtn" title="Go to top"><img class="arrow-img" src="arrow.png" alt="" srcset=""></button>
<button onclick="topFunction()" id="go-to-top-arrow" title="Go to top"><img class="arrow-img" src="arrow.png" alt="" srcset=""></button>

This should have a more descriptive name than myBtn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes Done !

js/guides.js Outdated
Comment on lines 104 to 105
document.body.scrollTop = 0;
document.documentElement.scrollTop = 0;
Copy link
Member

Choose a reason for hiding this comment

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

You can also use the scrollTo function: http://developer.mozilla.org/en-US/docs/Web/API/Window/scrollTo

Extra benefit: smooth scroll, so it's not a very sudden jump when you click the arrow.

Suggested change
document.body.scrollTop = 0;
document.documentElement.scrollTop = 0;
window.scrollTo({
top: 0,
left: 0,
behavior: "smooth",
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tombruijn Thank You soo much for optimizing my code I got to learn a lot from you. i have done the changes

index.html Outdated
@@ -29,6 +29,8 @@ <h2>Guides in other languages</h2>
</div>
</div>

<button onclick="topFunction()" id="myBtn" title="Go to top"><img class="arrow-img" src="arrow.png" alt="" srcset=""></button>
Copy link
Member

Choose a reason for hiding this comment

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

This location only makes it visible on the main page. Is it supposed to only be on the main page or every page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should be on every page. do I need to add this code manually on every page ?

@tombruijn tombruijn changed the title Update #539 Add "go back to top" arrow Oct 18, 2023
@VaibhavSharma24
Copy link
Contributor Author

Okay First of all thank you soo much @tombruijn for reviewing my code and taking out your precious time I have noted all the changes that you mentioned and I am on it.

@tombruijn tombruijn mentioned this pull request Oct 26, 2023
js/guides.js Outdated
}else{
$(".go-to-top-arrow").removeClass("active");
}
})

Choose a reason for hiding this comment

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

Missing semicolon.

js/guides.js Outdated
});
}

window.addEventListener("scroll", () => {

Choose a reason for hiding this comment

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

'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').

@VaibhavSharma24
Copy link
Contributor Author

@tombruijn Hello just letting you know that I have done the mentioned changes by you and have also added some features like transition to enhance user experience please let me know if there is anything in which I can help out

@tombruijn tombruijn merged commit bb56bce into railsgirls:main Oct 27, 2023
1 check passed
@tombruijn
Copy link
Member

Thanks for the PR @VaibhavSharma24! I've merged it.
I did some minor fixes after merging that you can find in this Git diff.

@AmanSingh81757 AmanSingh81757 mentioned this pull request Oct 27, 2023
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

3 participants