Resolve "Add and edit custom Wiki sidebar support from the UI"
What does this MR do?
Implements #23109 (closed)
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides - [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
mentioned in issue #23109 (closed)
added 1st contribution Community contribution labels
added Category:Wiki devopscreate + 1 deleted label
assigned to @toupeira
@toupeira could you help with the review?
8 8 = link_to git_access_url, class: active_nav_link?(path: 'wikis#git_access') ? 'active' : '', data: { qa_selector: 'clone_repository_link' } do 9 9 = icon('cloud-download', class: 'append-right-5') 10 10 %span= _("Clone repository") 11 11 = link_to git_access_url, class: active_nav_link?(path: 'wikis#_sidebar#edit') ? 'active' : '', data: { qa_selector: 'edit_sidebar_link' } do 12 = sprite_icon('cloud-download', size: 16, css_class: 'gl-mr-2') 13 %span= _("Edit Sidebar") @lifrank1994 there's a few things we need to change here:
-
git_access_url
is not the correct path yet, we can usewiki_page_path(@wiki, Wiki::SIDEBAR, action: :edit)
. -
active_nav_link?
doesn't work here as it can only detect the current controller and action, but not which wiki page we're editing. We also can't rely on the action as theedit
template can also be rendered by theshow
andupdate
actions. I think the easiest way around this would be to just pass the current view name fromedit.html.haml
. - The
cloud-download
icon doesn't exist in our SVG sprites, you can see all available icons at https://gitlab-org.gitlab.io/gitlab-svgs and I think herepencil
orpencil-square
would make sense. (for context: theicon
helper is legacy code and uses Font Awesome, if you rebasemaster
you'll see that we recently swapped the existingicon
call above to usesprite_icon
as well).
So putting it all together, I came up with this which seems to work as expected:
diff --git i/app/views/shared/wikis/_sidebar.html.haml w/app/views/shared/wikis/_sidebar.html.haml index cddf19fbc8e..9fb4261152d 100644 --- i/app/views/shared/wikis/_sidebar.html.haml +++ w/app/views/shared/wikis/_sidebar.html.haml @@ -1,3 +1,5 @@ +- current_view ||= nil + %aside.right-sidebar.right-sidebar-expanded.wiki-sidebar.js-wiki-sidebar.js-right-sidebar{ data: { "offset-top" => "50", "spy" => "affix" } } .sidebar-container .block.wiki-sidebar-header.gl-mb-3.w-100 @@ -9,6 +11,12 @@ = sprite_icon('download', size: 16, css_class: 'gl-mr-2') %span= _("Clone repository") + - edit_sidebar_path = wiki_page_path(@wiki, Wiki::SIDEBAR, action: :edit) + - edit_sidebar_class = (current_view == 'edit' && @page&.slug == Wiki::SIDEBAR) ? 'active' : '' + = link_to edit_sidebar_path, class: edit_sidebar_class, data: { qa_selector: 'edit_sidebar_link' } do + = sprite_icon('pencil-square', size: 16, css_class: 'gl-mr-2') + %span= _("Edit Sidebar") + .blocks-container .block.block-first.w-100 - if @sidebar_page diff --git i/app/views/shared/wikis/edit.html.haml w/app/views/shared/wikis/edit.html.haml index 64a4816def6..4149b6b821c 100644 --- i/app/views/shared/wikis/edit.html.haml +++ w/app/views/shared/wikis/edit.html.haml @@ -24,4 +24,4 @@ = render 'shared/wikis/form', uploads_path: wiki_attachment_upload_url -= render 'shared/wikis/sidebar' += render 'shared/wikis/sidebar', current_view: 'edit'
-
@lifrank1994 oh and we also need to wrap the whole thing in a permission check
So the link should only be visible if the user actually has permission to edit.The code for that is
can?(current_user, :create_wiki, @wiki.container)
, but that's a lot of logic in the view now so we should probably extract it into a helper method.@toupeira what does this line do with the view?
= render 'shared/wikis/sidebar', current_view: 'edit'
what does this line do with the view?
= render 'shared/wikis/sidebar', current_view: 'edit'
@lifrank1994 this defines a local variable
current_view
in theshared/wikis/sidebar
template. We also need to initialize the variable with- current_view ||= nil
, to make sure we don't get an exception on other views.changed this line in version 3 of the diff
8 8 = link_to git_access_url, class: active_nav_link?(path: 'wikis#git_access') ? 'active' : '', data: { qa_selector: 'clone_repository_link' } do 9 9 = icon('cloud-download', class: 'append-right-5') 10 10 %span= _("Clone repository") @lifrank1994 let's use title-casing here as well for consistency:
You'll need to run
rake gettext:regenerate
afterwards to update the translations inlocale/gitlab.pot
, this will also add the newEdit Sidebar
string.changed this line in version 2 of the diff
Changed this @toupeira
@tomquirk the icons are currently not well aligned, I'm wondering if there's an easy fix for that:
The layout also breaks when you shrink the window:
Maybe we should just have each link on their own line for now?
(not sure how to actually do that properly, I just quickly tested by wrapping both in a
<div>
)/cc @mnearents
@toupeira here is what I came up with!
Feel free to ping me for frontend review if you need it eventually (and nice work so far @lifrank1994
)Full-width responsive diff --git a/app/views/shared/wikis/_sidebar.html.haml b/app/views/shared/wikis/_sidebar.html.haml index 24b4ad98ddd..5d2251614f6 100644 --- a/app/views/shared/wikis/_sidebar.html.haml +++ b/app/views/shared/wikis/_sidebar.html.haml @@ -3,14 +3,15 @@ .block.wiki-sidebar-header.gl-mb-3.w-100 %a.gutter-toggle.float-right.d-block.d-sm-block.d-md-none.js-sidebar-wiki-toggle{ href: "#" } = sprite_icon('chevron-double-lg-right', size: 16, css_class: 'gl-icon') - - - git_access_url = wiki_path(@wiki, action: :git_access) - = link_to git_access_url, class: active_nav_link?(path: 'wikis#git_access') ? 'active' : '', data: { qa_selector: 'clone_repository_link' } do - = icon('cloud-download', class: 'append-right-5') - %span= _("Clone repository") - = link_to git_access_url, class: active_nav_link?(path: 'wikis#_sidebar#edit') ? 'active' : '', data: { qa_selector: 'edit_sidebar_link' } do - = sprite_icon('cloud-download', size: 16, css_class: 'gl-mr-2') - %span= _("Edit Sidebar") + + .gl-display-flex.gl-flex-wrap + - git_access_url = wiki_path(@wiki, action: :git_access) + = link_to git_access_url, class: 'gl-display-flex gl-align-items-center gl-white-space-nowrap gl-mr-5' + (active_nav_link?(path: 'wikis#git_access') ? ' active' : ''), data: { qa_selector: 'clone_repository_link' } do + = sprite_icon('download', size: 16, css_class: 'gl-mr-2') + %span= _("Clone repository") + = link_to git_access_url, class: 'gl-display-flex gl-align-items-center gl-white-space-nowrap' + (active_nav_link?(path: 'wikis#_sidebar#edit') ? ' active' : ''), data: { qa_selector: 'edit_sidebar_link' } do + = sprite_icon('pencil-square', size: 16, css_class: 'gl-mr-2') + %span= _("Edit sidebar") .blocks-container .block.block-first.w-100 - if @sidebar_page
assigned to @lifrank1994
@lifrank1994 I updated the MR description with our default template, there are a few things there we need to take care of:
- Adding a changelog entry: https://docs.gitlab.com/ee/development/changelog.html
- Adding tests, we could add them in https://gitlab.com/gitlab-org/gitlab/blob/eb2bd98b5a3ac9933a2535842baa91811dbf0922/spec/features/projects/wiki/user_creates_wiki_page_spec.rb#L304
- Updating the docs, I think we should mention this in https://docs.gitlab.com/ee/user/project/wiki/#customizing-sidebar. A screenshot would also be nice, but let's wait with that until the design is final
To build a test
gitlab/spec/helpers/wiki_helper_spec.rb
Want to create context of user. Look around for something like this
allow(helper).to receive(:can?).with(user, :read_cross_project).and_return(true)
Edited by Frank L@toupeira could you tell me the command you use to run gdk on a single thread again? Seems like I didn't write it down anywhere
@lifrank1994 that's
gdk thin
, and the docs for this are on https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/master/doc/howto/pry.md
added 2 commits
added backend label
@lifrank1994 hey Frank, I wanted to check in what your status is on this
There's still a few open discussions that need to be addressed, let me know if I can help or anything is unclear!
Hey @toupeira I'm still intending to continue working on this. Just that I'm starting a new job soon and I've been a little occupied wrapping helping the current company I'm working for document and prepare for my departure. I expect to be more free in two weeks and I was hoping to get this done on a weekend.
Are you folks hoping to have this done by a particular time? I can make sure that it's done in time for that!
Edited by Frank L@lifrank1994 ah ok no worries, there's absolutely no pressure from our side! Just wanted to check if you're blocked on something.
@lifrank1994 hey Frank, was wondering if you still wanted to work on this? If not we'll pick it up for %13.5, as we have quite a lot of wiki changes for this release and this would be a great little addition
Hey @toupeira! Do you think we could do a call sometime and finish it up together? Sorry to have delayed so much :)
@lifrank1994 I'm currently pretty distracted with lots of other things for the %13.5 release, so would prefer to do it async, or postpone again into %13.6
Let me know if you need any pointers, I think most of the open comments above still apply.
I also see now that the MR diff is currently showing both files as having changes on all lines, this is probably due to different line ending formats (Windows traditionally uses CRLF /
\r\n
, while other OSes use LF /\n
). It could be either an editor setting or your Git configuration that is causing this: https://docs.github.com/en/github/using-git/configuring-git-to-handle-line-endings@lifrank1994 I'll be off next week but would have time for a call after that (starting November 2). Would that work for you?
@lifrank1994 hey I thought I'd ping you again
Let me know if you want to have a call to finish this!@lifrank1994 that time works for me! Could you do tomorrow or Thursday? Otherwise I'll be off Friday, but Mo-Fr next week would work for me to.
Feel free to set up a call at https://calendly.com/mkoller.
Rather 7:50 am Eastern just to give me some time to wake up
@lifrank1994 ok great, sent you an invite! Please double-check that I got the time zone right
added sectiondev label
mentioned in issue #17347 (closed)
added groupeditor [DEPRECATED] label and removed 1 deleted label
Closing in favour of !50323 (merged)
mentioned in merge request !50323 (merged)