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

Issue#2155 ux #2171

Closed
wants to merge 3 commits into from
Closed

Issue#2155 ux #2171

wants to merge 3 commits into from

Conversation

saminarp
Copy link
Contributor

Description

With the following changes in client/app/components/Header/Header.scss, the navbar will be sticky.

More Details

The fixed navbar is not supported in mobile view. I intentionally left it out because the background is transparent in the navbar (i.e, background: transparent;) Giving the navbar a subtle color would prevent any clashes from occurring as a result of scrolling.

Corresponding Issue

This enhancement addresses the issue #2155.

Screenshots

Current behavior: navbar is in fixed position even with the scoll.

CleanShot 2022-10-13 at 05 05 29@2x

The screenshot below is the background color I would propose using but haven't implemented yet.

This would prevent the clashes with logo and content on the scroll
CleanShot 2022-10-13 at 05 02 41@2x


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

@welcome
Copy link

welcome bot commented Oct 13, 2022

Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking this on 🎉

@@ -97,6 +97,12 @@
}

&Desktop {
position: fixed;
Copy link
Member

Choose a reason for hiding this comment

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

With the header being fixed, you'll want to add some kind of margin-top value to the content of the page so that the header doesn't cover things. It should be height of the header + the existing margin or padding top value.

In addition, I think we should have a transparent background colour, I recommend using $black-80 or $black-90.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will be working on it! I will reach out via Slack even if I have questions.

@saminarp
Copy link
Contributor Author

Hello, I made a mistake force pushing the changes, please do not merge it. I will be creating a separate branch and push the updated PR

@saminarp
Copy link
Contributor Author

Please refer to this PR and ignore the previous

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

2 participants