[UI] Fix Navbar not closing on click getting started in mobile view #1889
Conversation
|
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Newcomers' Guide and sure to join the community Slack. |
✅ Deploy Preview for mesheryio-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Gemfile.lock
Outdated
| ffi (1.17.0-arm64-darwin) | ||
| ffi (1.17.0-x64-mingw-ucrt) | ||
| ffi (1.17.0-x86_64-darwin) | ||
| ffi (1.17.0-x86_64-linux-gnu) |
There was a problem hiding this comment.
Any reason for these changes?
There was a problem hiding this comment.
@hymmns revert these changes.
|
It would also be good to keep the animation we already have when closing the navbar menu. |
|
Ok thanks @amitamrutiya |
Gemfile.lock
Outdated
| ffi (1.17.0-arm64-darwin) | ||
| ffi (1.17.0-x64-mingw-ucrt) | ||
| ffi (1.17.0-x86_64-darwin) | ||
| ffi (1.17.0-x86_64-linux-gnu) |
There was a problem hiding this comment.
@hymmns revert these changes.
_includes/navigation.html
Outdated
| </nav> | ||
| <script> |
There was a problem hiding this comment.
move the js script to javascript file not in html file.Keep things seaparate
There was a problem hiding this comment.
@hymmns thoughts on this feedback? Will you be incorporating?
We don't need javascript to accomplish this. We can keep this behavior within CSS.
There was a problem hiding this comment.
At least with respect to the function in this file, we can do it within CSS.
There was a problem hiding this comment.
Hi @sudhanshutech and @leecalcote.
Does the changes address your comments.
Please let me know if you have any further requests
hymmns
force-pushed
the
hymmns/issue#1888
branch
2 times, most recently
from
912ac85
to
62f6082
Compare
js/stellarnav.js
Outdated
| toggleMenu(e); | ||
| window.location.href = e.target.href; |
There was a problem hiding this comment.
I'm not certain why e.preventDefault() was originally added, so to avoid causing any breaking changes, I introduced window.location.href = e.target.href to ensure navigation to the intended URL works
|
@amitamrutiya and @sudhanshutech does this address your comments |
|
Whoa @hymmns, you did an excellent job! One more request: if you can figure out how to do this, could you please add a smooth scrolling effect when we press the "Getting Started" button, regardless of the screen size? |
|
I found an issue in your deploy preview URL: when I press the "Getting Started" button on a large screen, all of the header options disappear. |
|
@hymmns, Thank you for your contribution! Let's discuss this during the website call on Monday at 5:30 PM IST (7:00 AM CT). Please consider adding it as an agenda item to the meeting minutes || meeting link. |
Signed-off-by: hymmns <[email protected]>
Signed-off-by: hymmns <[email protected]>
Signed-off-by: hymmns <[email protected]>
hymmns
force-pushed
the
hymmns/issue#1888
branch
from
80baf5e
to
0f5ad5f
Compare
|
@amitamrutiya, I have made the changes as requested. |
Signed-off-by: hymmns <[email protected]>
|
@hymmns, Join slack please. |
|
Thanks for the invite @Ashparshp |
|
Thank you for your contribution! Let's discuss this during the website call on Monday at 5:30 PM IST (7:00 AM CT). Please consider adding it as an agenda item to the meeting minutes || meeting link. |
|
@hymmns, I have added this as an agenda item to the meeting minutes. Please join the evening call. |
|
@hymmns what did we conclude with respect to this PR? |
|
Thanks @leecalcote, Will do |
Signed-off-by: hymmns <[email protected]>
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
issue/stale
Description
This PR fixes #1888
Notes for Reviewers
Signed commits