-
Notifications
You must be signed in to change notification settings - Fork 324
Start batch editing from the current page #303
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
Start batch editing from the current page #303
Conversation
@AonanLi is attempting to deploy a commit to the Sam Becker's projects Team on Vercel. A member of the Team first needs to authorize it. |
Been thinking about doing this for a while—thanks for looking into it! I would think choosing batch edit from |
Good catch! I also included other pages that don't show the grid. Choosing batch edit from pages like Another thought is to hide the batch edit button for those pages. It can be done but I'm slightly leaning towards the current approach. Thank you for building this awesome project! |
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.
Looks great. Left a few final comments.
|
||
// Check for batch editing parameter on mount | ||
useEffect(() => { | ||
if (searchParams.get('batch') === 'true') { |
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.
Sry it's a bit hacky, but I didn't find a better way yet without compromising the requirements or having a big refactor. This whole batch editing thing is more complicated than I expected.
I had to make this change, otherwise the Command K batch editing button from /full
will redirect users to /grid
without enabling batch editing (because of the useEffect in AdminAppMenu
).
The other options are:
- hide
Batch Editing ...
from Command K when the current page doesn't have a grid. - not clear batch editing when open
/grid
page. - refactor to move
AdminBatchEditPanel
and related state into the grid component. Seems complicated and risky. - refactor to use
<a href/>
sometimes instead of<Command.Item />
fromcmdk
.
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.
Agreed, this doesn't seem right.
What's the core issue here? Deciding whether or not to redirect from CMD-K menu? And how that interacts with the other useEffect
?
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.
We have two conflicting requirements for batch editing behavior:
- EXIT batch editing mode when user navigates to different pages
- Batch editing should be page-specific, not persistent across navigation
- Prevents confusing state where user is in batch mode on pages without grid photos
- Prevents invisible selections: if user selects 2 photos from /tag/a then navigates to /tag/b, those 2 photos remain selected even though they're not visible on the current page, creating confusing UX
- ENABLE batch editing when user clicks batch edit button on non-grid pages
- Button or Command K should redirect to /grid and activate batch editing mode
- This requires batch editing to persist across this specific navigation
So when we click batch edit on non-grid pages, it's trying to persist batch editing and also clear it at the same time.
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.
Ok that makes sense.
I wonder if we can set a flag (in state or just a non-reactive ref) before the non-user-derived navigation from the CMD-K menu so that particular navigation does not clear selection state?
Happy the build that last part if it seems too messy. Just LMK. Thank you for all the work thus far!
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.
Yeah that's kind of what this batch
search param is doing. It's a temporary flag to force enabling the selection state. Do we want to merge it? I'm also happy reverting that change and you can build the last part.
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.
Do we want to merge it?
Yes! I'll make that final tweak. Don't worry about reverting changes. Thank you again!
Description
Currently
Batch Edit
always redirects users to the home page. IMO it feels more intuitive to allow batch editing from the current page. If we don't want to tweak the existing button behavior, I can add an environment variable so users can opt in.