Skip to content

Replace dso page edit buttons with a menu#1852

Merged
tdonohue merged 31 commits intoDSpace:mainfrom
atmire:w2p-94390_replace-dso-page-edit-buttons-with-a-menu
Feb 10, 2023
Merged

Replace dso page edit buttons with a menu#1852
tdonohue merged 31 commits intoDSpace:mainfrom
atmire:w2p-94390_replace-dso-page-edit-buttons-with-a-menu

Conversation

@YanaDePauw
Copy link
Copy Markdown
Contributor

Description

This PR removes the separate buttons from the various DSpace object pages, such as the edit button and the versioning button, and puts them in a menu which will be rendered as buttons.

To add new buttons to the edit pages, these can be added to the menus in the DSOEditMenuResolver, similar to the general MenuResolver.

Every top level menu will be rendered as a button. When this menu has no child menus, clicking the button will perform the action defined in the menu section in the resolver. When the menu has child menus, clicking the button will show a dropdown containing the actions defined in the child menu sections.

Furthermore, because the version button on the item page can be disabled, the menu functionality was expanded with a disabled option. Support for this disabled property was added to all other existing menus, but is currently not in use.

Instructions for Reviewers

The changes were made in such a way that the UI remains the same for the different dspace object pages.
To verify this, you can do the following:

  • As an anonymous user or a user that does not have edit rights:
    • Navigate to some community, collection and item pages
    • Verify that you do not see any edit buttons
  • As an admin user or a user authorised to edit dspace objects:
    • Navigate to some community, collection and item pages
    • Verify that you can see the edit button
    • Verify that you can see the item version button on the item page and that it behaves as before

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@artlowel artlowel added improvement component: administrative tools Related to the admin menu or tools labels Sep 22, 2022
@artlowel artlowel added this to the 7.4 milestone Sep 22, 2022
@tdonohue tdonohue self-requested a review September 22, 2022 15:01
@tdonohue
Copy link
Copy Markdown
Member

@YanaDePauw : This has merge conflicts. Could you rebase this on main when you get the chance?

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Sep 27, 2022

This pull request introduces 1 alert when merging 6671bb3 into 064dae2 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ybnd
Copy link
Copy Markdown
Member

ybnd commented Sep 28, 2022

@tdonohue Sorry for pushing more changes this late, but we wanted to sneak in

  • Support for optional icons within dropdown menus
  • A fix for the width of the dropdown (it used to be very narrow, which caused issues for links with long titles)

Here's a quick branch demonstrating the dropdowns: w2p-94390_replace-dso-page-edit-buttons-with-a-menu_Testing

…' into w2p-94390_replace-dso-page-edit-buttons-with-a-menu
@ybnd ybnd force-pushed the w2p-94390_replace-dso-page-edit-buttons-with-a-menu branch from 5f817ac to 9dc7b34 Compare September 28, 2022 17:05
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Sep 28, 2022

This pull request introduces 2 alerts when merging 9dc7b34 into 695ce3a - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@tdonohue tdonohue removed this from the 7.4 milestone Oct 3, 2022
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks @YanaDePauw ! This all looks good to me now. I'm not noticing any changes in behavior via testing these menus & accessibility scans are now passing.

I agree with @artlowel above that I'd prefer to enhance these menus to align with others in DSpace 7 prior to more advanced changes/configurations. So, @atarix83 while I'm not against considering further enhancements later on, I think those would obviously need to wait for 7.6 or later to discuss. Unless we see major flaws with this PR, I'd rather we still consider it for 7.5 inclusion.

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YanaDePauw @tdonohue

I've reviewed again, and it overall looks good. The only thing I'd change is the orientation of the buttons in the menu. Personally i prefer an horizontal alignement instead of the current vertical one

Schermata da 2023-01-17 15-51-34

@tdonohue
Copy link
Copy Markdown
Member

@atarix83 : I believe the alignment is horizontal unless there's a very long title. That's the same behavior as we currently have on main. See that same item in the demo site: https://demo7.dspace.org/entities/publication/0ef86789-838c-424e-9585-0f917fbfea6a After logging in to the demo site, the buttons will appear vertical in the same way as your screenshot. However, if you visit an item with a shorter title, the button alignment will be horizontal.

So, as far as I can tell, I believe the behavior in this PR is identical to the behavior on main. Buttons are horizontal, unless the title is so long that it forces them to be vertical.

@atarix83
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown

Hi @YanaDePauw,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 7, 2023

Hi @YanaDePauw,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 9, 2023

Hi @YanaDePauw,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tdonohue tdonohue merged commit 9246e5e into DSpace:main Feb 10, 2023
@ybnd ybnd mentioned this pull request Mar 10, 2023
8 tasks
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Jun 21, 2024
[DSC-1751] Hide the altmetric label when there is no data available

Approved-by: Francesco Molinaro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: administrative tools Related to the admin menu or tools improvement

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

7 participants