-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Added ability to bulk delete locations #14304
Conversation
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
…ated work to be done Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
This pull request has been linked to Shortcut Story #24018: Figure out a way to conditionally select checkboxes with select-all, deselect-all. |
PR Summary
|
Signed-off-by: snipe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent change, and because it was BIG I was a little bit rigorous as I went through in review. Every change here seems warranted, and good, and good for the future and good for the product. This is some very excellent work, and I know it was painstaking and hard, and I think all of that effort paid off. Thank you, and I definitely approve.
This PR is a bit bigger and messier than I'd like, but as I was testing this, I found a few inconsistencies and bugs elsewhere. Sorry, @uberbrady - I know it's going to make this much harder to review.
I added some JS to selectively allow checkboxes to be checkable. I want this to be a LOT more nuanced, but there are some UI challenges with how bulk actions currently work that will make that a little tricksy. For example, the way I had originally done this, I added a sort of selectability object to the API response, which we could potentially use to determine which checkboxes should be selectable and which ones shouldn't. Problem is, the context is important, and we need to change that selectability based on the bulk actions dropdown options we present. This PR just covers bulk deleting locations, but down the line, I want this behavior to be a lot more ubiquitous for bulk actions. It gets complicated because without knowing the users' intents, we can't really decide what should be checkable or not checkable. I still think it can be done, but it started to get way out of scope for this PR, which is already a bit out of scope for what I wanted to accomplish.
isDeletable()
model method for locations. We were not accounting for child locations when deciding whether or not a location was deletableisDeletable()
method, which should make this easier to maintain moving forwardtrans_choice()
for strings and cut down on duplicated (or almost duplicated) strings. This should make life a lot easier on our translators.@return
statements that were incorrect or incompletemanager_id
to the locations API filter, which then let me switch us over to using the API for "Managed Locations" within the user detail view (no more client-side sorting, which speeds up page load quite a bit)Screen.Recording.2024-02-20.at.5.45.00.PM.mov
User View Before
User View After
Screen.Recording.2024-02-20.at.5.46.58.PM.mov