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

fixed #155: back to top button #197

Merged
merged 7 commits into from
Nov 2, 2023
Merged

fixed #155: back to top button #197

merged 7 commits into from
Nov 2, 2023

Conversation

mansoorbarri
Copy link
Contributor

Fixes #155

Screenshot:
image

@netlify
Copy link

netlify bot commented Oct 22, 2023

Deploy Preview for gokarna-hugo ready!

Name Link
🔨 Latest commit d8db256
🔍 Latest deploy log https://app.netlify.com/sites/gokarna-hugo/deploys/654295ae77720a0008eb12ff
😎 Deploy Preview https://deploy-preview-197--gokarna-hugo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yashmehrotra
Copy link
Collaborator

@mansoorbarri Thanks for the PR

Can you also:

  1. Feature gate it with the ShowBackToTopButton param
  2. Add the feature in theme-basic-documentation

@mansoorbarri
Copy link
Contributor Author

mansoorbarri commented Oct 23, 2023

@mansoorbarri Thanks for the PR

Can you also:

  1. Feature gate it with the ShowBackToTopButton param
  2. Add the feature in theme-basic-documentation

added

Copy link
Owner

@526avijitgupta 526avijitgupta left a comment

Choose a reason for hiding this comment

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

Hi @mansoorbarri thanks for submitting the PR. This feature would be quite useful :)

While the feature currently works, it abruptly takes the user to the top of the page. I would prefer if there is a smooth srolling to the top of the page. What I mean is it should work similar to how clicking on an item in the Table of Contents (on the right side of the page) works. @yashmehrotra wdyt?

Here's a code reference of how to smooth scrolling is implemented currently: https://github.com/526avijitgupta/gokarna/blob/be5611d4f02d679b43e869c1a542ecbf84f0febc/assets/js/main.js#L68C1-L78C2

Comment on lines 630 to 640
display: none; /* Hidden by default */
position: fixed; /* Fixed/sticky position */
bottom: 20px; /* Place the icon at the bottom of the page */
right: 30px; /* Place the icon 30px from the right */
z-index: 99; /* Make sure it does not overlap */
background-color: transparent; /* Remove the background color */
color: var(--accent-color); /* Icon color */
cursor: pointer; /* Add a mouse pointer on hover */
padding: 10px; /* Adjust the padding as needed */
border-radius: 50%; /* Make it a circle */
font-size: 24px; /* Increase font size */
Copy link
Owner

Choose a reason for hiding this comment

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

  1. please remove comments
  2. use z-index 5 (to match the z-index value of header element)
  3. order alphabetically
.arrow-logo {
  background-color: transparent;
  border-radius: 50%;
  bottom: 20px;
  color: var(--accent-color);
  cursor: pointer;
  display: none;
  font-size: 24px;
  padding: 10px;
  position: fixed;
  right: 30px;
  z-index: 5;
}

@mansoorbarri
Copy link
Contributor Author

appreciate the feedback. Please have a look now

Copy link
Collaborator

@yashmehrotra yashmehrotra left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@526avijitgupta Can you have a final look and merge this ?

@526avijitgupta
Copy link
Owner

@mansoorbarri This seems to break on mobile. . Could you please double check on your side and fix it before we merge?

Otherwise if someone builds Gokarna on the latest version and tries to use it on mobile screens it wouldn't be a good user experience for them

Also, let's use an icon which doesn't have a vertical line at at all. Basically, similar to an inverted "V"

@yashmehrotra
Copy link
Collaborator

Yeah, we should either pad it in way it does not come over text or disable it for mobile

image

@mansoorbarri
Copy link
Contributor Author

mansoorbarri commented Oct 26, 2023

I find this a bit alright but removing the button on mobile is probably the best option in my opinion. Do let me know what you would like.

Dark Light
image image

preview: https://gokarna.mansoorbarri.com

@526avijitgupta
Copy link
Owner

@mansoorbarri Thanks for making the changes.
In the current state the button looks okay, but we can certainly make it more visually appealing by making it match the site theme (light/dark).

I was browsing Hugo themes and came across this as an example: https://hugoloveit.com/emoji-support/#

Notice how the button's color and background color matches the current theme (light/dark).

Screenshot_20231026-233909.png

Screenshot_20231026-233934.png

@526avijitgupta
Copy link
Owner

Basically, what I mean is the blue background color on the back to top button looks a little out of place IMO. Simply changing that to match the light/dark theme should also be enough :)

@mansoorbarri
Copy link
Contributor Author

definitely, my first idea was to make it the same colour as code blocks on the website but I wasn't sure about that.

preview: https://gokarna.mansoorbarri.com

they look something like this:

Dark Light
image image

@mansoorbarri
Copy link
Contributor Author

mansoorbarri commented Oct 26, 2023

we can also make it a bit opaque but if you have any other ideas, do let me know.

@yashmehrotra
Copy link
Collaborator

@mansoorbarri @526avijitgupta

I think we should disable this on mobile and create a different issue to track that till we come up with a better design.
Lets try to get this PR merged with mobile disabled

What do you all think ?

@mansoorbarri
Copy link
Contributor Author

I agree

@mansoorbarri
Copy link
Contributor Author

the most we can do is move the button a bit towards right but it still comes over some of the page content:
image

@yashmehrotra
Copy link
Collaborator

the most we can do is move the button a bit towards right but it still comes over some of the page content:

@mansoorbarri Yeah, that is why I don't want to add this in mobile till we come up with a better solution

But I like adding a background color with the arrow

Lets:

  1. Disable it for mobile
  2. Add background color with arrow with just the inverted v

@526avijitgupta
Copy link
Owner

@mansoorbarri @526avijitgupta

I think we should disable this on mobile and create a different issue to track that till we come up with a better design.

@yashmehrotra Agreed!

@mansoorbarri
Copy link
Contributor Author

mansoorbarri commented Oct 30, 2023

the most we can do is move the button a bit towards right but it still comes over some of the page content:

@mansoorbarri Yeah, that is why I don't want to add this in mobile till we come up with a better solution

But I like adding a background color with the arrow

Lets:

  1. Disable it for mobile
  2. Add background color with arrow with just the inverted v

added! preview at https://gokarna.mansoorbarri.com

@yashmehrotra
Copy link
Collaborator

image

Can we make the padding in such a way that the V is in the middle, right now its left-centered

@mansoorbarri
Copy link
Contributor Author

I tried multiple changes to the CSS code however it still stays left aligned, the svg doesn't have a seperate div so I don't know why it shows up like that

right: 20px;
z-index: 5;
background-color: var(--light-tertiary-color);
border-radius: 10px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Border radius is twice, lets make this a complete circle

@yashmehrotra
Copy link
Collaborator

@mansoorbarri Doing padding-right: 8px makes it look like its at the centre
Also, lets make this a complete circle

@mansoorbarri
Copy link
Contributor Author

@yashmehrotra all changes are made, preview: https://gokarna.mansoorbarri.com/

@yashmehrotra
Copy link
Collaborator

@mansoorbarri Looks good

@526avijitgupta Should we merge this PR now ?

@526avijitgupta 526avijitgupta merged commit c5feccf into 526avijitgupta:main Nov 2, 2023
4 checks passed
@mansoorbarri mansoorbarri deleted the BTT-btn branch November 2, 2023 10:35
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.

Animated back to top button with CSS
3 participants