-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added navigation bar component #74
Added navigation bar component #74
Conversation
caf4ce1
to
7903464
Compare
5fc7159
to
1b950dc
Compare
LCOV of commit
|
7f6b45e
to
3795730
Compare
5bfb2b0
to
fd5587b
Compare
fd5587b
to
48e69e0
Compare
48e69e0
to
9bc1c82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a lot of feedback, but it was also a lot of code, so all in all very good (especially structured ...) and only some suggestions to improve the design.
One thing I noticed in general: I think the semantics of the back button are not correct. I am always redirected to the homepage or project overview and not back to the last page.
Furthermore, I think it would be nice to have some short comments at the beginning of each component, which describes what it is and where it is used.
src/lib/components/composites/navigation-bar/SimpleNavigationBar.svelte
Outdated
Show resolved
Hide resolved
src/lib/components/composites/navigation-bar/NavigationBar.svelte
Outdated
Show resolved
Hide resolved
9bc1c82
to
3fe71af
Compare
Thank you, for taking the time to review this PR! Let's discuss the navigation again, there are some open questions that have to be answered. |
bb1b087
to
abd9e31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only a few small things left, but I'd like to hear your opinion again, otherwise I think we're done.
One last question regarding the back navigation: do you create an issue for this problem or should I or would you like to solve it in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review again!
I'd like to this discuss the back button behavior in person.
…es to /components/composites
This makes the navigation bar component less clustered
abd9e31
to
1a09784
Compare
1a09784
to
4420f83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are finally done with it ...
Closes #73