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

UX: add bulk-select to mobile topic lists #15386

Merged
merged 7 commits into from Jul 26, 2022
Merged

Conversation

awesomerobot
Copy link
Member

@awesomerobot awesomerobot commented Dec 21, 2021

This will make bulk-select topic-list actions available on mobile. We were previously hiding bulk select on mobile due to space constraints.

This adds a new nav button:

Screen Shot 2022-04-12 at 12 59 40 PM

When the above nav button is clicked, avatars in the topic list are replaced by checkboxes, the bulk select bar sticks on scroll:

Screen Shot 2022-04-12 at 12 59 32 PM

@CvX
Copy link
Contributor

CvX commented Apr 8, 2022

@awesomerobot I'm sorry I let this fall through!

Imo it makes sense to do it this way. 👍

In the future we can consider using something like a mix of Ember's in-element helper and our plugin outlets, so that a component could "donate" a button to e.g. d-navigation. Something like:

{{!-- topic-list.hbs --}}
{{#extend-component "d-navigation"}}
  {{d-buttonsomething}}
{{/extend-component}}

…the rest of component

But for now this is completely fine. 😃

(to make up for the delay I rebased it to include latest main code and added a minor cleanup commit 😉)

@awesomerobot awesomerobot changed the title attempt to add bulk select to mobile topic lists UX: add bulk-select to mobile topic lists Apr 12, 2022
@awesomerobot awesomerobot marked this pull request as ready for review April 12, 2022 21:56
Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

This is really nice. It looks good to me, only holding off approval because

  • it's a slightly stale branch, we should rebase again and do a quick local test.
  • it would be great if we could add a minimal test, would be the same as topic-bulk-actions-test but using the mobile layout.

Nice work @awesomerobot!

@SamSaffron
Copy link
Member

This is a great change, any chance we can drive this to completion this week?

@awesomerobot
Copy link
Member Author

@pmusaraj @SamSaffron should be good to go now!

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

Looks great, just a few very minor comments. Thanks!

@awesomerobot awesomerobot merged commit b5c1132 into main Jul 26, 2022
@awesomerobot awesomerobot deleted the mobile-bulk-select branch July 26, 2022 19:36
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/no-way-to-select-multiple-messages-for-bulk-action-on-mobile-view/69996/17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants