-
Notifications
You must be signed in to change notification settings - Fork 90
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
Feature/470/split appnavigationitem into small components #486
Feature/470/split appnavigationitem into small components #486
Conversation
e40198d
to
6467b56
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.
This PR doesn't look like it is finished, but the tags and review requests say so. Hence, I did a fast review on the code, but I didn't test it in practice. This has to be done later.
Yep, sorry.. I just created it because I needed a bit of feedback/guidance (which you provided)! Thank you! |
f2c1914
to
46039cd
Compare
src/components/AppNavigationPinnedItems/AppNavigationPinnedItems.vue
Outdated
Show resolved
Hide resolved
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.
Just some small things, see comments.
Edit: And the performance issue is still there.
3fcb09b
to
ff0d1e1
Compare
Thanks @korelstar for yr thorough review! |
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
ff0d1e1
to
6106e77
Compare
@korelstar all clear and rebased! I'd say let's merge and individually fix the tiny issues we could find + do a proper aria pass on it! |
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.
🎉
Good thing! Last remaining drawback is the slow performance compared to the situation before this PR. I strongly suggest to fix this before the next release.
Could you perform a lighthouse? Or a performance audit? |
I can try this. Honestly, I never did this before. Which tool do you suggest? |
default brwoser tool. Either lighthouse or the profiling tab on chroomium or performance on firefox :) |
I've created a new issue for the performance problem: #577 |
Fix #470
Fix #376
Please use this App.vue in you vueexample app for reviewing, as the new components won't work with the old one!