Skip to content
Snippets Groups Projects

Draft: Vue super sidebar PoC

Closed Paul Gascou-Vaillancourt requested to merge super_vue_sidebar_poc into master
5 unresolved threads

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, which GlDropdown does, but GlListbox doesn't.
  • Still related to the user dropdown: rendering a GlToggle within a GlDropdown 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 with GlMenu.
  • 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, some a 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)?
Edited by Paul Gascou-Vaillancourt

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • A deleted user added backend frontend labels

    added backend frontend labels

  • 15 Errors
    :no_entry_sign: 521a86cd: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 031bb6eb: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 45676b96: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 480cfb39: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 749e6dce: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 3ef518a1: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: e0c1a50e: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 3e0d0610: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: ab0fe42a: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 02d67a83: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: cb1a777e: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 8e5f21a1: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: c7c9c431: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: 0e142980: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
    :no_entry_sign: This MR has more than 20 commits. You need to rebase this branch to have fewer commits.
    15 Warnings
    :warning: This merge request is quite big (766 lines changed), please consider splitting it into multiple merge requests.
    :warning: This merge request includes more than 10 commits. Each commit should meet the following criteria:
    1. Have a well-written commit message.
    2. Has all tests passing when used on its own (e.g. when using git checkout SHA).
    3. Can be reverted on its own without also requiring the revert of commit that came before it.
    4. 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.

    :warning: 031bb6eb: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines.
    :warning: 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.
    :warning: 45676b96: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 3ef518a1: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines.
    :warning: 491bcc9c: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 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.
    :warning: 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.
    :warning: This merge request changed files with disabled eslint rules. Please consider fixing them.
    :warning: This merge request contains deprecated components. Please consider using Pajamas components instead.
    :warning: This merge request changed frontend files without pretty printing them.
    :warning: Please add a merge request subtype to this merge request.
    :warning: Please add a merge request type to this merge request.
    :warning: This merge request does not refer to an existing milestone.
    1 Message
    :book: 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 current availability (@avielle) (UTC+1, 6 hours ahead of @pgascouvaillancourt) Alexandru Croitor current availability (@acroitor) (UTC+2, 7 hours ahead of @pgascouvaillancourt)
    frontend Axel García current availability (@agarciatesares) (UTC-3, 2 hours ahead of @pgascouvaillancourt) Mike Greiling current availability (@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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 3 commits

    Compare with previous version

  • added 1 commit

    • 3e0d0610 - wip: add user info and urls for dropdown

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Thomas Hutterer changed the description

    changed the description

  • Thomas Hutterer added 3 commits

    added 3 commits

    • 3ef518a1 - add sidebar css class to placeholder element
    • 749e6dce - wip: start with new sidebar menus
    • 480cfb39 - show menu icons

    Compare with previous version

    • Bundle size analysis [beta]

      This compares changes in bundle size for entry points between the commits e2d8b3fc and 521a86cd

      :sparkles: Special assets

      Entrypoint / 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 %

      :tada: Significant Reduction: 290

      Expand
      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 the bundle-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 :no_entry_sign: Danger

    • This 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 :thinking:

      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 Florian
    • Hmm. For some reason, it looks like CI_MERGE_REQUEST_SOURCE_BRANCH_SHA wasn't defined :thinking:

      Edited by Mark Florian
    • Please register or sign in to reply
  • added 1 commit

    Compare with previous version

  • @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 :boom:

  • added 1 commit

    Compare with previous version

  • added 2 commits

    • d3248a63 - Abort icon rendering if icon name not available
    • 3026d099 - Mock user dropdown

    Compare with previous version

  • Paul Gascou-Vaillancourt changed the description

    changed the description

  • added 1 commit

    • 026a1587 - Use actual links in user menu

    Compare with previous version

  • Paul Gascou-Vaillancourt changed the description

    changed the description

  • Paul Gascou-Vaillancourt changed the description

    changed the description

  • Thomas Hutterer added 2 commits

    added 2 commits

    • 6fdf5dfe - Show the current menu item as active
    • dfe069a9 - Show menu which contains active item as expanded

    Compare with previous version

  • added 1 commit

    • 031bb6eb - fix: take action_name from controller method

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 521a86cd - hack! use super sidebar also for groups

    Compare with previous version

    • @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 :boom: 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!
    • Thanks for those neat changes @thutterer :bow: I moved the code to app/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.

    • Please register or sign in to reply
  • added 1 commit

    • d1a6d648 - Move super sidebar code to a more generic location

    Compare with previous version

  • added 1 commit

    • 2591aad4 - Move super sidebar code to a more generic location

    Compare with previous version

    • @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! :tada: 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! :nerd: 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 :fingers_crossed:

    • Please register or sign in to reply
  • 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, but GlListbox 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 menu

    Hmm, I would have thought it should just be a disclosure :thinking:

    See gitlab-ui#1631 (comment 1129877319) for some discussion about the unlikely need for GlMenu.

  • Mark Florian mentioned in commit 0dcb1092

    mentioned in commit 0dcb1092

  • Mark Florian mentioned in merge request !104620 (closed)

    mentioned in merge request !104620 (closed)

  • Mark Florian added 1 commit

    added 1 commit

    • 5c4de864 - Allow disabling startup_css for speedtest comparison

    Compare with previous version

  • added 1 commit

    • fc1c9f5d - Revert changes from earlier debugging

    Compare with previous version

  • mentioned in merge request !107540 (merged)

  • Please register or sign in to reply
    Loading