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

Select all feature added. #495

Merged
merged 6 commits into from
Feb 19, 2020
Merged

Conversation

biggiebangle
Copy link

Description of the Change

Related to #469 and #393
A "Select All" button has been added above the "Search Available Connections" search bar. This "Select All" is disabled after initial page load, if no connections are available for syndication.

Alternate Designs

n/a

Benefits

If there are a large amount of connections to syndicate, time will be saved with the addition of a select all button.

Possible Drawbacks

n/a

Verification Process

Testing scenarios
1.
Select All
-remove some
-send
Select All
-send remaining
2.
Select All
-remove some
-put some back
-send
Select All
-send remaining
3.
Select Some
-send
Select All
-send
4.
Select Some
-remove some
-send
Select All
-send
5.
Select Some
-remove some
-send
Select All
-remove some
-send
Select All
-send

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

@jeffpaul jeffpaul requested a review from dkotter December 5, 2019 18:15
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Dec 5, 2019
@jeffpaul jeffpaul removed the request for review from dkotter December 5, 2019 18:16
@jeffpaul jeffpaul added this to the Future Release milestone Dec 5, 2019
@jeffpaul
Copy link
Member

jeffpaul commented Dec 5, 2019

@biggiebangle welcome to Distributor and thanks for the PR! Per the checklist/task item in #393 (include Select all and None link options), could you also look to include a None link option as well to undo someone selecting the Select All link option?

@biggiebangle
Copy link
Author

Hello @jeffpaul! Added a None option as requested. Hope this finds you well, and please let me know if you need anything else.

@jeffpaul jeffpaul modified the milestones: Future Release, 2.0.0 Dec 9, 2019
@jeffpaul jeffpaul requested a review from dkotter December 9, 2019 17:29
@jeffpaul
Copy link
Member

jeffpaul commented Dec 9, 2019

@biggiebangle thanks for the update, we'll get this reviewed and let you know if we have any additional questions or concerns... thanks!

@dkotter
Copy link
Collaborator

dkotter commented Dec 13, 2019

@biggiebangle Thanks for the PR! Testing this out, works well.

Couple suggestions though. When no connections are selected, can we make the None button have a disabled look? And similarly, when all connections are selected, give the Select All button a disabled state?

Basically just want to give a visual indication that those buttons won't do anything in those states (i.e. if no connections are selected, clicking None won't do anything, so giving that a disabled look makes that more clear). Not exactly sure what this state should look like, could be a normal disabled button look or could also use the secondary-button styling that WordPress core uses.

We would need to add this disabled state to the None button when no connections are selected. So essentially on initial load and then listen for connections to be removed and when the last one is removed, disable the button again.

And same for the Select All. When all connections are selected, disable that button but as soon as one connection is removed, activate it again (this is the trickier one, as we need to determine when all connections are chosen. Maybe comparing the number of items in the available connections list to the ones in the selected list and if they match, that means all are selected?)

And then a bigger issue shows up if you have lots of connections. Right now, if there are more than 5 connections, we add a search input and add scroll overflow on our connections list:
Screen Shot 2019-12-13 at 4 14 01 PM

But if you add too many connections to distribute to at once, the selected list doesn't have that overflow scroll, so it can end up breaking the layout:
Screen Shot 2019-12-13 at 4 14 32 PM
I think ideally we would constrain the selected list to a certain height and then have an overflow scroll after that point (so you can still scroll through and see all selected connections).

Let me know if you have any questions on any of these. Thanks!

@biggiebangle
Copy link
Author

Hello dkotter! I added the CSS that I am using on our child plugin to address this overflow issue. I am out of office for the next few weeks for the holiday, but I will check out the rest of your suggestions then.

@dkotter
Copy link
Collaborator

dkotter commented Jan 27, 2020

@biggiebangle Hi! I see you've pushed a few code changes since your last update, but just wanted to check in and see if you have more work you're wanting to do here before I review this again? Thanks!

@biggiebangle
Copy link
Author

I think we are good to go. Let me know if you need anything else!

@dkotter
Copy link
Collaborator

dkotter commented Feb 14, 2020

@biggiebangle Thanks! This looks really good. I think this is good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants