Draft: Vue super sidebar PoC
Notes
- The user dropdown probably needs to be implemented with
GlDropdown
for now. Ideally, this would be implemented as a menu, but we don't have a component for this yet, perhaps something we should prioritize as part of this effort. It's worth noting that the dropdown's toggle needs to be the user's avatar, which implies having control over that area via a slot, whichGlDropdown
does, butGlListbox
doesn't. - Still related to the user dropdown: rendering a
GlToggle
within aGlDropdown
proved to be cumbersome as the latter's styles are leaking into the former. We'll need to be careful about this sort of thing withGlMenu
. - The PoC currently leverages the existing
.nav-sidebar
CSS class to piggy back on the current spacing and positioning. In the actual implementation, we might want to start from scratch to avoid undesired styles from getting in the way. For example, somea
styles are leaking in the user dropdown menu, we've had to override the left margin manually there. - Potential startup CSS caveat: gitlab-org/frontend/rfcs#92 (comment 1169303552).
- Empty aside while the Vue app initializes: would this be a good candidate for UI over the Wire (gitlab-org/frontend/rfcs#112 - closed)?
Merge request reports
Activity
assigned to @pgascouvaillancourt and @thutterer
15 Errors 521a86cd: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 031bb6eb: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 45676b96: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 480cfb39: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 749e6dce: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 3ef518a1: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. e0c1a50e: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 3e0d0610: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. ab0fe42a: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 02d67a83: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. cb1a777e: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 8e5f21a1: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. c7c9c431: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 0e142980: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. This MR has more than 20 commits. You need to rebase this branch to have fewer commits. 15 Warnings This merge request is quite big (766 lines changed), please consider splitting it into multiple merge requests. This merge request includes more than 10 commits. Each commit should meet the following criteria: - Have a well-written commit message.
- Has all tests passing when used on its own (e.g. when using git checkout SHA).
- Can be reverted on its own without also requiring the revert of commit that came before it.
- Is small enough that it can be reviewed in isolation in under 30 minutes or so.
If this merge request contains commits that do not meet this criteria and/or contains intermediate work, please rebase these commits into a smaller number of commits or split this merge request into multiple smaller merge requests.
031bb6eb: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines. b49aefb4: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 45676b96: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 3ef518a1: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines. 491bcc9c: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 02d67a83: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 0e142980: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. This merge request changed files with disabled eslint rules. Please consider fixing them. This merge request contains deprecated components. Please consider using Pajamas components instead. This merge request changed frontend files without pretty printing them. Please add a merge request subtype to this merge request. Please add a merge request type to this merge request. This merge request does not refer to an existing milestone. 1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Disabled eslint rules
The following files have disabled
eslint
rules. Please consider fixing them:app/assets/javascripts/super_sidebar/components/context_switcher.vue
app/assets/javascripts/super_sidebar/components/super_sidebar.vue
Run the following command for more details
node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \ 'app/assets/javascripts/super_sidebar/components/context_switcher.vue' \ 'app/assets/javascripts/super_sidebar/components/super_sidebar.vue'
Deprecated components
These deprecated components are in the process of being migrated. Please consider using Pajamas components instead.
- <button
Pretty print Frontend files
The following files should have been pretty printed with
prettier
:app/assets/javascripts/main.js
Please run
node_modules/.bin/prettier --write \ 'app/assets/javascripts/main.js'
Also consider auto-formatting on-save.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Avielle Wolfe (
@avielle
) (UTC+1, 6 hours ahead of@pgascouvaillancourt
)Alexandru Croitor (
@acroitor
) (UTC+2, 7 hours ahead of@pgascouvaillancourt
)frontend Axel García (
@agarciatesares
) (UTC-3, 2 hours ahead of@pgascouvaillancourt
)Mike Greiling (
@mikegreiling
) (UTC-6, 1 hour behind@pgascouvaillancourt
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger @pgascouvaillancourt - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request.
- Resolved by Thomas Hutterer
added 3 commits
added grouppersonal productivity sectiondev labels
added devopsmanage label
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits e2d8b3fc and 521a86cd
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent mainChunk 1.96 MB 2.11 MB +157.73 KB 7.9 % average 3.6 MB 3.7 MB +93.77 KB 2.5 % Significant Reduction: 290Expand
Entrypoint / Name Size before Size after Diff Diff in percent pages.projects.incidents.show 4.66 MB 4.53 MB -137.98 KB -2.9 % pages.projects.issues.designs 4.68 MB 4.54 MB -137.98 KB -2.9 % pages.projects.issues.edit 4.61 MB 4.48 MB -137.98 KB -2.9 % pages.projects.issues.new 4.61 MB 4.48 MB -137.98 KB -2.9 % pages.projects.issues.service_desk 4.53 MB 4.4 MB -137.98 KB -3.0 % pages.projects.issues.show 4.68 MB 4.54 MB -137.98 KB -2.9 % pages.projects.merge_conflicts 2.59 MB 2.45 MB -137.98 KB -5.2 % pages.projects.merge_requests.conflicts 2.5 MB 2.37 MB -137.98 KB -5.4 % pages.projects.merge_requests.index 2.75 MB 2.61 MB -137.98 KB -4.9 % pages.projects.merge_requests.show 5.58 MB 5.45 MB -137.98 KB -2.4 % The table above is limited to 10 entries. Please look at the full report for more details
Note: We do not have exact data for e2d8b3fc. So we have used data from: 06f18a29.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun thebundle-size-review
job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerThis is interesting. It looks like most pages reduced by 137.98 KB, while the main chunk increased by 157.73 KB.
I'm guessing this is due to certain GitLab UI components being moved from those chunks to the main chunk?
The net result is an average increase per page of 93.77 KB (sizes are uncompressed JS).
Having said all that, I'm not sure if this analysis is valid. e2d8b3fc is not a direct ancestor of 521a86cd. In fact there's quite a lot of divergence
Edit: Ah, I think maybe Pipeline For Merged results changes the logic of which commits are compared... Something to look into. Right, yes:
The target commit was too new, so we used the latest commit from master we have info on. It might help to rerun the bundle-size-review job
- I'll do that.Edited by Mark FlorianHmm. For some reason, it looks like
CI_MERGE_REQUEST_SOURCE_BRANCH_SHA
wasn't definedEdited by Mark Florian
@pgascouvaillancourt I've pushed a couple more commits:
3ef518a1 adds some CSS to the placeholder element, to have less flicker when the Vue app takes over. For now it's just the wrapper, not the top purple area yet.
749e6dce replaces all the old project sidebar menu contents with a new grouping. only simple "Plan" and "Develop" done for now, but we can build out the rest when we get more details about the groupings. The way I've done it leads to some duplicated code for all the child menu items that likely stay the same mostly, but it allows us to build this out without conflicing at all with the old sidebars.
480cfb39 just adds the icons for top-level menus.
45676b96 removes the topbar
- Resolved by Thomas Hutterer
added 2 commits
added 1 commit
- 031bb6eb - fix: take action_name from controller method
@pgascouvaillancourt I've pushed a couple more changes today:
- The Vue.js sidebar now shows the active menu item as
.active
and also has its parent menu expanded on page load. - I've added back the classic Settings to the new project menu, just to have a bit more to test the
.active
with. - Also groups use the new sidebar now
Note that this is just a quick hack. Please see the TODOS and FIXMEs in 521a86cd and, if you have time, please move the JS files out of "project" to a more generic place. I wasn't sure where would be best. Thanks!
- The Vue.js sidebar now shows the active menu item as
Thanks for those neat changes @thutterer
I moved the code toapp/assets/javascripts/super_sidebar/
, I think this should do the trick for now. I'm not yet sure about the "super" term, but it should be fine to not get the naming 100% right in the Poc.
added 1 commit
- d1a6d648 - Move super sidebar code to a more generic location
added 1 commit
- 2591aad4 - Move super sidebar code to a more generic location
@thutterer @pgascouvaillancourt I am looking for a real-world demo of gitlab-org/frontend/rfcs#112 (closed). Would the sidebar be simple enough to give it a shot? (any state management, Apollo, complex components in there?)
Edited by Hannes Moser@lamportsapprentice Hi Hannes, awesome idea!
We were just talking the other day how SSR would be great to have (or maybe even a must-have) when rendering the main nav with Vue.And your UI-over-the-wire approach looks super promising.
Please give it a shot!
Currently the sidebar is still a POC only so we are taking some shortcuts (and write no tests yet), so what you'll find might be messy, but overall it's still pretty simple.
The initial state is all read from a data attribute from the DOM, so no network roundtrips required, currently. And it doesn't manage any state between page loads yet.
Later on it might get more involved, as there are design ideas for mobile layouts where certain elements would move places, for example: the user counts that by default are in the sidebar header, would move into the user dropdown on a small screen. So there will probably be some dependency on the screen width and so on, but not at the moment.
Cool, and thanks for the swift reply. I am going to try and make it work with this PoC. We need some real-world examples to make a solid case for SSR (I mean, it is somewhat obvious, but still). This is going to help, and we should at least see perceived UX improvements (time to content, fewer glitches, …).
Yes, please!
In the current state of the POC the sidebar vue app "flashes in" on every page load. Could be worked around a bit with having a static sidebar shell with same shape and colors and some skeleton loaders, but way better would be to have the real thing ready with the rest of the rails-rendered content
mentioned in issue #381718 (closed)
It's worth noting that the dropdown's toggle needs to be the user's avatar, which implies having control over that area via a slot, which
GlDropdown
does, butGlListbox
doesn't.I've already got the question about whether we plan to support slot content for the toggle. It has not resulted in a formal issue but it confirms the need for the feature.
The user dropdown probably needs to be implemented with
GlDropdown
for now. Ideally, this would be implemented as a menuHmm, I would have thought it should just be a disclosure
See gitlab-ui#1631 (comment 1129877319) for some discussion about the unlikely need for
GlMenu
.mentioned in commit 0dcb1092
mentioned in merge request !104620 (closed)
added 1 commit
- 5c4de864 - Allow disabling startup_css for speedtest comparison
@pgascouvaillancourt Shall we close this MR as "draft done" or keep it open and make it into the real thing later, WDYT?
@thutterer I vote for closing it as is so that we can easily refer to it during the actual implementation
mentioned in merge request !107540 (merged)