LB-1759: Added a checkbox to make multiple listens or albums keeping the modal open #3269
Conversation
|
@amCap1712 if any changes needed do let me know |
There was a problem hiding this comment.
The mechanism I would expect as a user is this: after I checked the "add another" checkbox, when I click on the submit button it should clear the selected listen but keep the modal open.
It seems that this was your intention, but you'll need to dig a bit deeper as there are other interactions with sub-components .
Don't hesitate to ask me questions here that you might have.
| onClick={closeModal} | ||
| > | ||
| Close | ||
| </button> | ||
| <button | ||
| type="submit" | ||
| type="button" |
There was a problem hiding this comment.
Is this change required? Just out of curiosity.
There was a problem hiding this comment.
Yes, this change is needed. I changed it from type="submit" to type="button" because we don’t want the button to submit the form in the usual way, which could reload the page. Instead, the button just runs our own JavaScript function to handle everything. Using type="button" makes sure the button only does what we want and doesn’t accidentally submit the form or refresh the page. Also I am new to making pull requests ,so please let me know if there is anything i should improve
There was a problem hiding this comment.
I see! The way to ensure the page is not reloaded is to add use the event's preventDefault method in the submit callback.
A random example from the codebase:
listenbrainz-server/frontend/js/src/components/Navbar.tsx
Lines 25 to 26 in 03d06f4
Generally, keeping the type:submit is important for accessibility (for example for screen readers for visually impaired users).
The literature on accessibility guidelines is quite dry, but if you want more context here it is :) https://www.w3.org/WAI/WCAG21/Techniques/html/H32
There was a problem hiding this comment.
Hi @MonkeyDo , I just wanted to confirm a few things about my AddListenModal.tsx file after making the changes you suggested:
- I set
type="submit"for the submit button to make it more accessible. - I used
e.preventDefault()in the form handler to stop the page from reloading. - The actual submit logic is now inside the
submitListensfunction.
See my changed AddListenModal.tsx file :
import React, { useCallback, useContext, useState } from "react";
import DateTimePicker from "react-datetime-picker/dist/entry.nostyle";
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
import { IconProp } from "@fortawesome/fontawesome-svg-core";
import { faCalendar } from "@fortawesome/free-regular-svg-icons";
import NiceModal, { useModal } from "@ebay/nice-modal-react";
import { toast } from "react-toastify";
import { Link } from "react-router-dom";
import { add } from "date-fns";
import { get } from "lodash";
import GlobalAppContext from "../../utils/GlobalAppContext";
import { convertDateToUnixTimestamp } from "../../utils/utils";
import { ToastMsg } from "../../notifications/Notifications";
import AddSingleListen from "./AddSingleListen";
import AddAlbumListens from "./AddAlbumListens";
import Pill from "../../components/Pill";
enum SubmitListenType {
"track",
"album",
}
export type MBTrackWithAC = MusicBrainzTrack & WithArtistCredits;
export function getListenFromRecording(
recording: MusicBrainzRecordingWithReleasesAndRGs,
date?: Date,
release?: MusicBrainzRelease & WithReleaseGroup
): Listen {
const releaseOrCanonical = release ?? recording.releases[0];
const listen: Listen = {
listened_at: date ? convertDateToUnixTimestamp(date) : 0,
track_metadata: {
artist_name:
recording["artist-credit"]
?.map((artist) => `${artist.name}${artist.joinphrase}`)
.join("") ?? "",
track_name: recording.title,
release_name: releaseOrCanonical?.title,
additional_info: {
release_mbid: releaseOrCanonical?.id,
release_group_mbid: releaseOrCanonical?.["release-group"]?.id,
recording_mbid: recording.id,
submission_client: "listenbrainz web",
artist_mbids: recording["artist-credit"].map((ac) => ac.artist.id),
},
},
};
if (recording.length) {
// Cannot send a `null` duration
listen.track_metadata.additional_info!.duration_ms = recording.length;
}
return listen;
}
export const getListenFromTrack = (
track: MusicBrainzTrack & WithArtistCredits,
date: Date,
release?: MusicBrainzRelease
): Listen => {
const listen: Listen = {
listened_at: convertDateToUnixTimestamp(date),
track_metadata: {
track_name: track.title,
artist_name:
track["artist-credit"]
?.map((artist) => `${artist.name}${artist.joinphrase}`)
.join("") ?? "",
additional_info: {
recording_mbid: track.recording.id,
submission_client: "listenbrainz web",
track_mbid: track.id,
tracknumber: track.number,
artist_mbids: track["artist-credit"].map((ac) => ac.artist.id),
},
},
};
if (track.length) {
// Cannot send a `null` duration
listen.track_metadata.additional_info!.duration_ms = track.length;
} else if (track.recording.length) {
// Cannot send a `null` duration
listen.track_metadata.additional_info!.duration_ms = track.recording.length;
}
if (release) {
listen.track_metadata.additional_info!.release_mbid = release.id;
listen.track_metadata.release_name = release.title;
}
return listen;
};
// Use a default of 1 minute for the track length if not available in metadata
export const DEFAULT_TRACK_LENGTH_SECONDS = 60;
export default NiceModal.create(() => {
const modal = useModal();
const { APIService, currentUser } = useContext(GlobalAppContext);
const { auth_token } = currentUser;
const [listenOption, setListenOption] = useState<SubmitListenType>(
SubmitListenType.track
);
const [selectedListens, setSelectedListens] = useState<Listen[]>([]);
const [customTimestamp, setCustomTimestamp] = useState(false);
const [selectedDate, setSelectedDate] = useState(new Date());
const [invertOrder, setInvertOrder] = useState(false);
const [keepModalOpen, setKeepModalOpen] = useState(false);
// Used for the automatic switching and search trigger if pasting URL for another entity type
const [textToSearch, setTextToSearch] = useState<string>();
const closeModal = useCallback(() => {
modal.hide();
document?.body?.classList?.remove("modal-open");
const backdrop = document?.querySelector(".modal-backdrop");
if (backdrop) {
backdrop.remove();
}
setTimeout(modal.remove, 200);
}, [modal]);
const handleError = useCallback(
(error: string | Error, title?: string): void => {
if (!error) {
return;
}
toast.error(
<ToastMsg
title={title || "Error"}
message={typeof error === "object" ? error.message : error.toString()}
/>,
{ toastId: "add-listen-error" }
);
},
[]
);
const submitListens = useCallback(async () => {
if (auth_token) {
let payload: Listen[] = [];
const listenType: ListenType =
selectedListens?.length <= 1 ? "single" : "import";
// Use the user-selected date, default to "now"
const date = customTimestamp ? selectedDate : new Date();
if (selectedListens?.length) {
let orderedSelectedListens = [...selectedListens];
if (invertOrder) {
orderedSelectedListens = orderedSelectedListens.reverse();
}
let cumulativeDateTime = date;
payload = orderedSelectedListens.map((listen) => {
// Now we need to set the listening time for each listen
const modifiedListen = { ...listen };
const durationMS = get(
modifiedListen,
"track_metadata.additional_info.duration_ms"
);
const timeToAdd =
(durationMS && durationMS / 1000) ??
modifiedListen.track_metadata.additional_info?.duration ??
DEFAULT_TRACK_LENGTH_SECONDS;
// We either use the previous value of cumulativeDateTime,
// or if inverting listening order directly use the new value newTime
const newTime = add(cumulativeDateTime, {
seconds: invertOrder ? -timeToAdd : timeToAdd,
});
if (invertOrder) {
modifiedListen.listened_at = convertDateToUnixTimestamp(newTime);
} else {
modifiedListen.listened_at = convertDateToUnixTimestamp(
cumulativeDateTime
);
}
// Then we assign the new time value for the next iteration
cumulativeDateTime = newTime;
return modifiedListen;
});
}
if (!payload?.length) {
return;
}
const listenOrListens = payload.length > 1 ? "listens" : "listen";
try {
const response = await APIService.submitListens(
auth_token,
listenType,
payload
);
await APIService.checkStatus(response);
toast.success(
<ToastMsg
title="Success"
message={`You added ${payload.length} ${listenOrListens}`}
/>,
{ toastId: "added-listens-success" }
);
if (!keepModalOpen) {
closeModal();
} else {
// Reset the form state but keep the modal open
setSelectedListens([]);
setTextToSearch("");
}
} catch (error) {
handleError(
error,
`Error while submitting ${payload.length} ${listenOrListens}`
);
}
} else {
toast.error(
<ToastMsg
title="You need to be logged in to submit listens"
message={<Link to="/login/">Log in here</Link>}
/>,
{ toastId: "auth-error" }
);
}
}, [
auth_token,
selectedListens,
customTimestamp,
selectedDate,
invertOrder,
APIService,
closeModal,
handleError,
keepModalOpen,
]);
const switchMode = React.useCallback(
(pastedURL: string) => {
if (listenOption === SubmitListenType.track) {
setListenOption(SubmitListenType.album);
} else if (listenOption === SubmitListenType.album) {
setListenOption(SubmitListenType.track);
}
setTimeout(() => {
// Trigger search in the inner (grandchild) search input component by modifying the textToSearch prop in child component
// Give it some time to allow re-render and trigger search in the correct child component
setTextToSearch(pastedURL);
}, 200);
setTimeout(() => {
// Reset text trigger
setTextToSearch("");
}, 500);
},
[listenOption]
);
const userLocale = navigator.languages?.length
? navigator.languages[0]
: navigator.language;
const handleFormSubmit = (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
submitListens();
};
return (
<div
className={`modal fade ${modal.visible ? "in" : ""}`}
id="AddListenModal"
tabIndex={-1}
role="dialog"
aria-labelledby="AddListenModalLabel"
data-backdrop="static"
>
<div className="modal-dialog" role="document">
<form className="modal-content" onSubmit={handleFormSubmit}>
<div className="modal-header">
<button
type="button"
className="close"
data-dismiss="modal"
aria-label="Close"
onClick={closeModal}
>
<span aria-hidden="true">×</span>
</button>
<h4 className="modal-title" id="AddListenModalLabel">
Add Listens
</h4>
</div>
<div className="modal-body">
<div className="add-listen-header">
<Pill
active={listenOption === SubmitListenType.track}
onClick={() => {
setListenOption(SubmitListenType.track);
}}
type="secondary"
>
Add track
</Pill>
<Pill
active={listenOption === SubmitListenType.album}
onClick={() => {
setListenOption(SubmitListenType.album);
}}
type="secondary"
>
Add album
</Pill>
</div>
{listenOption === SubmitListenType.track && (
<AddSingleListen
onPayloadChange={setSelectedListens}
switchMode={switchMode}
initialText={textToSearch}
/>
)}
{listenOption === SubmitListenType.album && (
<AddAlbumListens
onPayloadChange={setSelectedListens}
switchMode={switchMode}
initialText={textToSearch}
/>
)}
<hr />
<div className="timestamp">
<h5>Timestamp</h5>
<div className="timestamp-entities">
<Pill
active={customTimestamp === false}
onClick={() => {
setCustomTimestamp(false);
setSelectedDate(new Date());
}}
type="secondary"
>
Now
</Pill>
<Pill
active={customTimestamp === true}
onClick={() => {
setCustomTimestamp(true);
setSelectedDate(new Date());
}}
type="secondary"
>
Custom
</Pill>
<div className="timestamp-date-picker">
<div>
<label htmlFor="starts-at">
<input
name="invert-timestamp"
type="radio"
checked={invertOrder === false}
id="starts-at"
aria-label="Set the time of the beginning of the album"
onChange={() => {
setInvertOrder(false);
}}
/>
Starts at:
</label>
<label htmlFor="ends-at">
<input
name="invert-timestamp"
type="radio"
checked={invertOrder === true}
id="ends-at"
aria-label="Set the time of the end of the album"
onChange={() => {
setInvertOrder(true);
}}
/>
Finishes at:
</label>
</div>
<DateTimePicker
value={selectedDate}
onChange={(newDateTimePickerValue: Date) => {
setSelectedDate(newDateTimePickerValue);
}}
calendarIcon={
<FontAwesomeIcon icon={faCalendar as IconProp} />
}
maxDate={new Date()}
clearIcon={null}
format={userLocale ? undefined : "yyyy-MM-dd hh:mm:ss"}
locale={userLocale}
maxDetail="second"
disabled={!customTimestamp}
/>
</div>
</div>
</div>
</div>
<div
className="modal-footer"
style={{ display: "flex", alignItems: "center", gap: "1rem" }}
>
<div style={{ flex: 1 }}>
<label style={{ userSelect: "none", cursor: "pointer" }}>
<input
type="checkbox"
checked={keepModalOpen}
onChange={(e) => setKeepModalOpen(e.target.checked)}
style={{ marginRight: "0.5em" }}
/>
Add another
</label>
</div>
<button
type="button"
className="btn btn-default"
onClick={closeModal}
>
Close
</button>
<button
type="submit"
className="btn btn-success"
disabled={!selectedListens?.length}
>
Submit {selectedListens.length ?? 0} listen
{selectedListens.length > 1 ? "s" : ""}
</button>
</div>
</form>
</div>
</div>
);
});
But I ran into a small issue with browser validation:
When I select tracks and try to submit, the browser still asks for the input field to be filled in (because it’s marked as required), even though I’ve already picked tracks.
see in the screenshot
One More thing it is running fine with Add Album see below :
To fix that, I made the input field required only if no tracks are selected.
In AddSingleListen, I added this:
requiredInput={selectedRecordings.length === 0}And in SearchTrackOrMBID, I used that value like this:
<input required={props.requiredInput} />After that, TypeScript gave me an error saying:
“Property 'requiredInput' is missing…”
I Think this is happening because now all places where I use SearchTrackOrMBID need to include the requiredInput prop.
So now I’m wondering:
- Should I go and update every usage of
SearchTrackOrMBIDand addrequiredInput={true}orfalse? - Or is it better to make
requiredInputoptional and give it a default value? - Which approach is the best practice?
Thank You for the help !!
There was a problem hiding this comment.
Nice work :)
This should all work fine, but I suggest a small stylistic change:
For the form submission, you don't need a new handleFormSubmit function, you can directly pass the submitListens function like so:
This was exactly correct, no need to change it:
<form className="modal-content" onSubmit={submitListens}>
Then modify the submitListens function based on the changes you already made:
const submitListens = useCallback(async (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
if (auth_token) {
[...]
All I did was, like you, define the expected event type, and add the prevenDefault call right before the rest of the function.
It's essentially the same as what you did, but combining it into submitListens instead of defining a new handleFormSubmit function in-between.
Regarding the required input, looks like I didn't think everything through ;)
But I still think this is a good accessibility change overall, so as you suggested the second option is the way to go:
"make requiredInput optional and give it a default value"
If you look in the SearchTrackOrMBID component this is how we deal with the autofocus prop:
listenbrainz-server/frontend/js/src/utils/SearchTrackOrMBID.tsx
Lines 43 to 55 in 7f44897
| setSelectedListens([]); | ||
| setTextToSearch(""); |
There was a problem hiding this comment.
I have the impression that this is not working as expected?
At least in the video in the PR description it shows two selected listens one under the other, when I would expect clicking the 'submit' button to clear the current selection
I think this is possibly because some of the logic is in the child components AddSingleListen and AddAlbumListens ?
There was a problem hiding this comment.
@MonkeyDo Thanks for catching this ! You are right the selected listens are not being fully cleared after submitting, which is why multiple listens are showing up in the modal. I think this is because, even though I’m resetting the selectedListens state in the parent, the child components AddSingleListen and AddAlbumListens might not be resetting their own state or syncing properly.
I’ll dig into how the child components manage their state and make sure everything resets as expected when “Add another” is checked. If you have any suggestions for the best way to reset the child components, I would love to hear them! Otherwise, I will update the PR soon with a fix. Thanks again for your helpful feedback
There was a problem hiding this comment.
At a quick glance I didn't find a good way to clear the child components' state.
The two ideas I have in mind:
- simple one: sometimes you can reset a child component's state by adding a
keyprop like we do here for example. I'm not sure that'll work or be efficient, but it might be worth a try? - Export a ref object from the AddSingleListen and AddAlbumListens components so that the parent can call a specific 'reset' method on the children. See for example how we use that in AddAlbumListens itself: follow the breadcrumbs for the
searchInputRef, defined here in the child withforwardRefand useImperativeHandle, assigned here in the parent and used to trigger an internal methodtriggerSearchhere:
listenbrainz-server/frontend/js/src/user/components/AddAlbumListens.tsx
Lines 110 to 119 in 03d06f4
Also see the react docs page for context: https://react.dev/reference/react/useImperativeHandle
There was a problem hiding this comment.
Forgot to paste this link about the key prop: https://react.dev/reference/react/useState#resetting-state-with-a-key
There was a problem hiding this comment.
hey @MonkeyDo I tried the first option to reset the form state by passing a key prop for child components AddSingleListen and AddAlbumListens and it is not working !! , so now I will go ahead with the second option !!!
There was a problem hiding this comment.
@MonkeyDo So After proceeding with the second option and doing required changes to related files this is the conclusion you can see in the video below :
Your.Listens.-.ListenBrainz.and.10.more.pages.-.Personal.-.Microsoft_.Edge.2025-05-16.04-19-33.online-video-cutter.com.mp4
So can I go ahead to merge the commit for this pr ?
And thankyou for your efforts you put in this to guide me step by step I really appreciate it !!
There was a problem hiding this comment.
Yes you can push the commit, I'll do another review.
nice work!
|
@MonkeyDo Thankyou for the review ,I'll look onto the suggested changes and queries tomorrow travelling right now 🙂!! |
…ks and albums) as per mentor feedback
|
@MonkeyDo I have pushed the new commits let me know if any changes needed commit |
| setSelectedAlbumMBID(undefined); | ||
| setSelectedAlbum(undefined); | ||
| setSelectedTracks([]); | ||
| searchInputRef.current?.triggerSearch(""); |
There was a problem hiding this comment.
I think this won't do anything as it is, as the triggerSearch imperative handle function calls setInputValue, and the useEffect hook that reacts to changes in inputValue returns if the string value is falsy (including an empty string):
listenbrainz-server/frontend/js/src/utils/SearchAlbumOrMBID.tsx
Lines 158 to 162 in 27577fd
I think I see in your video demo that the previous search results are still loaded in the dropdown after you submit a listen, and this is why.
However, there is already a reset function defined in there, which we can expose to the parent component with the same imperativehandle hook
listenbrainz-server/frontend/js/src/utils/SearchAlbumOrMBID.tsx
Lines 150 to 157 in 27577fd
This would mean moving the reset function definition higher up so it is defined before the imperativehandle hook, wrap it in a useCallback hook to avoid unnecessary re-renders (reset will have to be a dependency of the imperativehandle hook), and expose it to the parent.
All in all, like this:
const reset = useCallback(() => {
setInputValue("");
setSearchResults([]);
onSelectAlbum();
setLoading(false);
searchInputRef?.current?.focus();
}, [onSelectAlbum]);
// Allow parents to focus on input and trigger search
useImperativeHandle(
inputRefForParent,
() => {
return {
focus() {
searchInputRef?.current?.focus();
},
triggerSearch(newText: string) {
setInputValue(newText);
},
reset,
};
},
[reset]
);| @@ -47,6 +52,13 @@ export default function AddSingleListen({ | |||
| }; | |||
| }, [initialText]); | |||
|
|
|||
| useImperativeHandle(ref, () => ({ | |||
| reset: () => { | |||
There was a problem hiding this comment.
Along with my comment above, I think this is where you'll want to call the new function to reset the search component state
(searchInputRef?.current?.reset())
No worries at all I actually enjoyed working through the feedback and learned a lot in the process! I agree, these changes really do make the codebase more flexible and accessible. Thanks for all your guidance and for taking the time to review everything so thoroughly. |
|
@MonkeyDo I’ve updated the search components to expose the reset function through the imperative handle, and I’m now calling |
De-duplicate this type definition and move it to a type file. Define type of component in forwardRef calls for SearchTrackOrMBID and SearchAlbumOrMBID Use said type in components that use a ref to the controllable search input
There was a problem hiding this comment.
I hope you don't mind but I was pondering how to improve the typescript types situation with the changes you made, and thought I might as well commit the result once I got it working.
I deduplicated the type used in useRef and put it in a types file, then used it with the useRef and useImperativeHandle calls where they were needed.
@MonkeyDo Not at all, I really appreciate you taking the time to improve the TypeScript types! I have just tested your commit (the one where you moved the |
|
I deployed this to our test server for in-situ testing, everything works perfectly, nice work! Merging this now, some users are going to be happy about this :) |
Thank you @MonkeyDo ! I really enjoyed working on this and appreciate your support throughout. It was a great experience collaborating with you ,looking forward to contributing more! |
MonkeyDo
changed the title
Problem
LB-1759
The problem is when a user wants to add multiple listen in row (like for each side of a record ) or multiple albums with different timestamps for example ,so the user have to first submit the listen he is listening to and for adding another listen or album user had to again open the modal and add manually the listen or album he wants to, so this is very frustrating to open the modal again and again repeatedly, so the user wants an option which help him to keep the modal open so they can add multiple listens with very few clicks
Can see what was earlier :
Solution
I added a checkbox named "Add another" to the Add Listens modal in
AddListenModal.tsx. When a user tick this add another check box, the modal stays open after they submit a listen, so they can quickly add more without reopening it each time. If they leave it unchecked, the modal closes as usual. This makes adding multiple listens much faster and provides more control, similar to GitHub’s “Create more” option.I have created a checkbox as
The preview of the solution is in the video below :
Your.Listens.-.ListenBrainz.and.2.more.pages.-.Personal.-.Microsoft_.Edge.2025-05-09.23-19-40.online-video-cutter.com.mp4