Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-31038: As an editor, I want to perform more bulk actions on sub-items #219

Merged
merged 11 commits into from
Dec 20, 2019
Merged

EZP-31038: As an editor, I want to perform more bulk actions on sub-items #219

merged 11 commits into from
Dec 20, 2019

Conversation

ITernovtsii
Copy link
Contributor

@ITernovtsii ITernovtsii commented Oct 15, 2019

Question Answer
JIRA issue EZP-31038
Type Feature
Target version master
BC breaks no
Doc needed no

New bulk actions implemented: "Hide Locations", "Unhide Locations", "Add Locations"

Notes:

Copy link
Member

@bdunogier bdunogier left a comment

Choose a reason for hiding this comment

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

I like it, thank you for your contribution.
I wish we could refactor these operations into an extension point, but it would be a lot to ask (would it ?).

@ITernovtsii
Copy link
Contributor Author

thanks @bdunogier ,
sorry, I'm a bit over time with this so will work on changes slower than on implementation.

At the same time, I started development using extraActions property, at some point found that it's making code a bit overcomplicated (duplicate UDW, events to track & clear selected items).
And, I didn't find any benefits from it (except that it would be a good example of extending the component).
Is this what you mean as "an extension point"?

Copy link
Member

@dew326 dew326 left a comment

Choose a reason for hiding this comment

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

Just a few small comments but overall looks good.

Thanks for your contribution.

src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
ITernovtsii and others added 2 commits October 15, 2019 15:35
Co-Authored-By: Dariusz Szut <dew326@gmail.com>
@SylvainGuittard
Copy link

Thanks @ITernovtsiy for this great contribution!
@dew326 This PR is against master. Which version of eZ Platform do we target?

@SylvainGuittard
Copy link

@ITernovtsiy I am also curious to have some screenshots. // cc @inakijv for review

@ITernovtsii
Copy link
Contributor Author

@SylvainGuittard it looks like this
all-visible
both-visibility

@SylvainGuittard
Copy link

@ITernovtsiy Thanks for uploading the screenshots.
Just to confirm the behavior: if I select one visible content item and one hidden content item, the reveal button is active because of the single hidden item, right?

@ITernovtsii
Copy link
Contributor Author

Right, if both hidden and visible items selected, both buttons become active.

@dew326
Copy link
Member

dew326 commented Oct 16, 2019

@SylvainGuittard everything in the PR and ticket suggests it will be part of 3.0 (and as the semVer says new features in the new version, not in patches), but it's up to you if we want to make an exception for this one. I'm not against merging it to 2.5, we just need to make the config optional and add default empty value.

@bdunogier
Copy link
Member

Is this what you mean as "an extension point"?

Well, I guess. I don't have a strong opinion about the implementation, but in that case, it would be an API / set of configuration that allows to add a button that acts on selected locations:

  • in which circumstances the button is enabled
  • its icon/label
  • check for permissions
  • the action it executes
  • ...

@ITernovtsii
Copy link
Contributor Author

@bdunogier Agree, it would be nice to extend current extraActions to work as you described. But I'm from the backend world and will not be able to design it well enough on React side.
I think you can add it to the product board, and who knows, maybe someone (or myself) will have time to implement it.
Anyway, it will be a separate PR.

@bdunogier
Copy link
Member

But I'm from the backend world and will not be able to design it well enough on React side.

Haha, I have the same issue. In that case, even bigger kuddos to you for going into hostile unknown territory ! We, or somebody else, can take care of it.

@SylvainGuittard
Copy link

SylvainGuittard commented Oct 16, 2019

it's up to you if we want to make an exception for this one. I'm not against merging it to 2.5, we just need to make the config optional and add default empty value.

@dew326 let me check with the customer who was asking for this feature. I will let you know.

Copy link
Contributor

@DominikaK DominikaK left a comment

Choose a reason for hiding this comment

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

A few cosmetic changes to the labels.

Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
Resources/translations/sub_items.en.xliff Outdated Show resolved Hide resolved
ITernovtsii and others added 2 commits November 19, 2019 08:31
Co-Authored-By: DominikaK <dominika.kurek@ez.no>
Co-Authored-By: DominikaK <dominika.kurek@ez.no>
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/services/bulk.service.js Outdated Show resolved Hide resolved
Co-Authored-By: DominikaK <dominika.kurek@ez.no>
Copy link
Contributor

@DominikaK DominikaK left a comment

Choose a reason for hiding this comment

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

+1 for strings

Copy link
Member

@dew326 dew326 left a comment

Choose a reason for hiding this comment

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

Just small code style, in general +1.

src/modules/sub-items/sub.items.module.js Outdated Show resolved Hide resolved
src/modules/sub-items/sub.items.module.js Show resolved Hide resolved
Co-Authored-By: Dariusz Szut <dew326@gmail.com>
Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

Just one thing, using the X button does not close the reveal modal. See, please http://g.recordit.co/JiK4dZRBCJ.gif

@inakijv
Copy link
Contributor

inakijv commented Nov 28, 2019

@ITernovtsiy thank you for this contribution!
For the "Add new location" button, please add new icon.

location-add-new.svg.zip

@ITernovtsii ITernovtsii changed the base branch from master to 1.5 December 17, 2019 11:47
@ITernovtsii
Copy link
Contributor Author

icon pushed to ezsystems/ezplatform-admin-ui#1108

@ITernovtsii ITernovtsii reopened this Dec 17, 2019
@ITernovtsii ITernovtsii changed the base branch from 1.5 to master December 17, 2019 12:00
@ITernovtsii ITernovtsii changed the base branch from master to 1.5 December 17, 2019 12:18
@ITernovtsii
Copy link
Contributor Author

ITernovtsii commented Dec 17, 2019

rebased to v1.5, let me know if it should be master

@dew326
Copy link
Member

dew326 commented Dec 17, 2019

@ITernovtsiy As far as I know it was decided to merge it to the master (3.0)

@ITernovtsii
Copy link
Contributor Author

oops, I thought it was planned to merge into 2.x too.
Rebasing back to the master

@ITernovtsii ITernovtsii changed the base branch from 1.5 to master December 17, 2019 14:05
@ITernovtsii
Copy link
Contributor Author

Thanks @katarzynazawada, good catch!

Fixed copy-pasting typo in c7fda4a

@katarzynazawada
Copy link

@ITernovtsiy , closing Reveal modal works fine now.
But Add locations button has no background after change. It should have blue (ez-color-secondary) background, like other buttons.
Screenshot 2019-12-18 at 10 42 59

@lserwatka
Copy link
Member

@katarzynazawada could you retest it?

@lserwatka lserwatka merged commit fe16223 into ezsystems:master Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants