Skip to content
Snippets Groups Projects

Add a dropdown to filter and sort projects in the dashboard projects & group pages

Closed Add a dropdown to filter and sort projects in the dashboard projects & group pages
add-filter-sort-dropdown-to-dashboard-group-pages into master
Closed username-removed-128633 requested to merge add-filter-sort-dropdown-to-dashboard-group-pages into master

Addresses #3968 (closed), #3245 (closed), #3799 (closed). This MR is bigger than I expected but I don't think this could be split into smaller MRs.

/cc @creamzy @dzaporozhets @JobV @balameb @dblessing @haynes

Notes:

  • I replaced "Last updated" by "Recently active" since the underlying field used with this sorting method is last_activity_at which is the time of the most recent project's event. I thought it was not very useful to sort by updated_at (which is updated when the project settings itself is updated) because from a user perspective the goal is to see the most recently active projects, not the most recently updated.
  • I used the Project's personal scope for the "Owned by me" filter.
  • I did not show the "Owned by" filters in the group page since it doesn't make sense: group's projects are owned by the group.
  • The group page has been split in two real pages (instead of loading the activity feed and the project lists on one page) since sorting reloads the page, it was cleaner to do it like that. The two pages are /groups/:group -> group's activity (same as before) and /groups/:group/projects -> group's projects. The former /groups/:group/projects route has been changed to /groups/:group/projects/edit which is more appropriate IMO.

Dashboard > Your projects Owned by anyone sorted by Recently active

Screen_Shot_2016-01-27_at_12.46.13

Dashboard > Your projects Owned by me sorted by Most stars

Screen_Shot_2016-01-27_at_12.46.52

Dashboard > Starred projects Owned by anyone sorted by Name

Screen_Shot_2016-01-27_at_12.47.30

Group (while signed-out) > Activity

group-signed-out-activity

Group (while signed-out) > All projects sorted by Most stars

group-signed-out-projects

Group (while signed-in) > Activity

group-signed-in-activity

Group (while signed-in) > All projects sorted by Name

group-signed-in-projects

Group > No activity to show

group-no-activity

Group > No projects to see

group-no-projects

Explore > Update of the dropdown style (to match the style in the proposal's screenshots)

explore-dropdown-style-update

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
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 750aa82a - Fix specs, add specs, fix Rubocop offences, fix some queries & views
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 9fc53e6b - Fix specs and clean links in group page
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

  • username-removed-128633 Title changed from WIP: Add a dropdown to filter and sort projects in the dashboard & the group page to Add a dropdown to filter and sort projects in the dashboard projects & group pages

    Title changed from WIP: Add a dropdown to filter and sort projects in the dashboard & the group page to Add a dropdown to filter and sort projects in the dashboard projects & group pages

  • username-removed-128633 Reassigned to @DouweM

    Reassigned to @DouweM

  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    @jschatz1 You'll probably want to review this too since it's a Frontend MR!

  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    Re. the group page: I realize the changes in GroupsController and its views will clash with GitLab EE...

    I see two possibilities:

    1. Revert to keep using the "dynamic tab" approach (i.e. load activity and projects in the show action and show tabs in JS) as it is currently in master
    2. Keep the changes and handle the merge into EE with the new approach: add a new shared scope to the list of scopes in CE, i.e.: All (w) | Contributed (x) | Starred (y) | Shared (z). This solution could really be cleaner than the first one, IMHO.
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    @rymai

    Keep the changes and handle the merge into EE with the new approach: add a new shared scope to the list of scopes in CE, i.e.: All (w) | Contributed (x) | Starred (y) | Shared (z). This solution could really be cleaner than the first one, IMHO.

    I prefer this option, but we should get some UX/Frontend input :)

    I also think we should either split up Activity and Projects into different navigation items (like on the Dashboard, and Project where Activity has its own item), or at least make sure Group in the left is highlighted when on the Projects page. I prefer the first option since it's more consistent.

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    Overriding the dropdown styles locally will bite us when the UI framework comes along and we change the dropdown style across the board.

    @jschatz1 @creamzy What should we do here? Is there a new dropdown design that covers this case (a label like 'Sort by')?

  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • app/controllers/dashboard/milestones_controller.rb
    14 14 private
    15 15
    16 16 def projects
    17 @projects ||= current_user.authorized_projects.sorted_by_activity.non_archived
    17 @projects ||= current_user.authorized_projects.sort('recently_active').non_archived
    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      I prefer to use scopes here, so we can update the criteria without needing to search for every use, like we're doing now. You can simply redefine sorted_by_activity as sort('recently_active').

    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      Never mind, I missed that sort was overwritten! In that case this is OK!

  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • app/views/layouts/group_settings.html.haml
    1 page_title "Settings"
  • Douwe Maan
  • Douwe Maan
  • Douwe Maan Reassigned to @rymai

    Reassigned to @rymai

  • username-removed-128633 Added 1 commit:

    Added 1 commit:

  • Douwe Maan
  • Douwe Maan
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 70297375 - Fix specs and address latest MR feedback
  • Douwe Maan
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • config/routes.rb
    346 346 member do
    347 347 get :issues
    348 348 get :merge_requests
    349 get :projects
    350 349 end
    351 350
    352 351 scope module: :groups do
    352 resources :projects, only: [:index] do
    353 get :edit, on: :collection
    354 end
    353 355 resources :group_members, only: [:index, :create, :update, :destroy] do
  • Douwe Maan mentioned in issue #12779 (closed)

    mentioned in issue #12779 (closed)

  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 2bd80129 - Addressing latest MR feedbacks
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • d3a51414 - Ensure specs do not fail randomly
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 2221ed87 - Add Group > Activity page and in the group nav bar
  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 5d993ef9 - Fix groups/:id/activity route redirect
  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    @DouweM Thanks for the awesome review, I've pushed a few commits to add new "Activity" and "Projects" items in the group's nav bar (see screenshots in the MR description). If you can review them that'd be great!

    Also, regarding the known conflict that will happen with EE (with the "Shared projects" feature in group page), should I go ahead and open a MR for CE master -> EE master once this one gets merged into CE master?

    @creamzy & @jschatz1: It would be great if you could give your feedback about this MR as well. Thanks in advance!

    PS: I will merge master in that branch once it's approved to minimize the need for master merges.

  • username-removed-128633 Reassigned to @DouweM

    Reassigned to @DouweM

  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • 76d6b1c0 - Fix specs broken by last commit
  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    Also, regarding the known conflict that will happen with EE (with the "Shared projects" feature in group page), should I go ahead and open a MR for CE master -> EE master once this one gets merged into CE master?

    Shared projects will be backported to CE by #12831 (closed) so it would be wiser to wait for it to be in master before merging this MR so that I'll be able to resolve the conflicts during the master merge into this branch.

  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • c79c441c - Re-add code that didn't need to be removed
  • username-removed-128633 Added 258 commits:

    Added 258 commits:

    • c79c441c...22772871 - 257 commits from branch master
    • 264d25da - Merge remote-tracking branch 'origin/master' into add-filter-sort-dropdown-to-dashboard-group-pages
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • app/views/groups/_header.html.haml 0 → 100644
    13 .avatar-holder
    14 = link_to group_icon(@group), target: '_blank' do
    15 = image_tag group_icon(@group), class: "avatar group-avatar s90"
    16 .cover-title= @group.name
    17
    18 .cover-desc.username= @group.to_reference
    19
    20 - if @group.description.present?
    21 .cover-desc.description
    22 = markdown(@group.description, pipeline: :description)
    23
    24 %ul.nav-links
    25 = nav_link(path: 'groups#activity') do
    26 = link_to 'Activity', activity_group_path(@group)
    27 = nav_link(path: 'groups/projects#index') do
    28 = link_to 'Projects', group_projects_path(@group)
    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      Hmm. I don't like the duplication of the Activity/Projects tabs and nav items, I would prefer to not have the tabs, but that's out of scope for this MR. Would you mind reverting the sidebar change, so we just have a Group item there that's highlighted when we're on the Activity or Projects tab?

    • username-removed-128633
      username-removed-128633 @rymai ·
      Author Maintainer

      Hmm. I don't like the duplication of the Activity/Projects tabs and nav items, I would prefer to not have the tabs, but that's out of scope for this MR.

      I'm not a fan of that neither, but as you said since it was not part of the MR I thought that could be a first step on which we could iterate on (it was weird to have the group header without the tab underneath for both the Activity and the Projects page).

      Anyways, that was a try but I will definitely be more comfortable not changing the sidebar in this MR so that UX team can come up with a consistent proposal. I'll revert that last changes.

  • username-removed-128633 Added 1 commit:

    Added 1 commit:

    • d984beba - Revert splitting group into "Activity" / "Projects" in the group nav bar
  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    @DouweM I've addressed your last comment, thanks! I'll assign this MR to @jschatz1 now.

  • username-removed-128633 Reassigned to @jschatz1

    Reassigned to @jschatz1

  • Jacob Schatz
    Jacob Schatz @jschatz1 ·
    Maintainer

    @rymai Nice job! Functionally it looks like it works.

    1. #3968 (closed) It does not match the new dropdowns.
    2. Use <li role="separator" class="divider"></li> not <hr>. Don't reinvent the wheel please.
    3. http://getbootstrap.com/components/#dropdowns-headers Don't reinvent the wheel please.

    I think we need a plan for the new dropdowns, I think this regular bs dropdown works here, but it obviously won't work in other places. Do you need me to style these for you or can you handle it?

  • Jacob Schatz Reassigned to @rymai

    Reassigned to @rymai

  • username-removed-444
    username-removed-444 @dzaporozhets ·
    Owner

    @rymai this merge request is huge and tries to fix too much problem at once. I don't think anyone can efficiently review 1.5K changed files. I strongly recommend you to split this merge request on smaller one each solving small particular problem. cc @DouweM

  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    @dzaporozhets I agree, this definitely got a little out of hand. Thank you for your kind warning. :)

    1. I propose to start with adding sort only on the dashboard/projects& dashboard/projects/starred with the current dropdown UI.
    2. Then I (or someone else) should update the dropdown UI according to #3279 (closed).
    3. At that time, we'll have the UI ready to add the filtering to dashboard/projects & dashboard/projects/starred.
    4. Only at that point should we start thinking about adding sorting/filtering to the group page (and see if we split it in two distinct pages, as I've done in this MR).

    Let me know if that sounds like a (good) plan to you, or not! ;)

    If you're ok with at least 1., I will open a new MR for it (once !2677 (merged) and !2678 (merged) are merged since it will avoid many conflicts).

  • username-removed-444
    username-removed-444 @dzaporozhets ·
    Owner

    Let me know if that sounds like a (good) plan to you, or not! ;)

    @rymai awesome plan. Each step -> at least one merge request

    I will open a new MR for it (once !2677 (merged) and !2678 (merged) are merged since it will avoid many conflicts).

    smart move!

  • username-removed-444
    username-removed-444 @dzaporozhets ·
    Owner

    @rymai With !2677 (merged) introduced I will provide an example by moving existing dropdown on dashboard. I will link MR here and will mention you. Using provided semantic you can proceed with your plan

  • username-removed-128633
    username-removed-128633 @rymai ·
    Author Maintainer

    Awesome, thanks @dzaporozhets!

  • username-removed-444 mentioned in merge request !2691 (merged)

    mentioned in merge request !2691 (merged)

  • username-removed-444 mentioned in commit c8224f0b

    mentioned in commit c8224f0b

  • username-removed-128633 Status changed to closed

    Status changed to closed

  • username-removed-408881 mentioned in issue #3799 (closed)

    mentioned in issue #3799 (closed)

    • Please register or sign in to reply