-
Notifications
You must be signed in to change notification settings - Fork 28
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
835 extend caption editing area UI #837
Conversation
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.
Changing the DOM is strangel.
(From Chatgpt): Directly manipulating ref.current.innerText might conflict with React's reconciliation process. This could cause React to lose sync with the DOM.
Improvement: If possible, manage text updates via state or props rather than directly mutating the DOM:
const [innerText, setInnerText] = useState(originalValue);
useEffect(() => {
if (ref.current) ref.current.innerText = innerText;
}, [innerText]);
if (ref.current) { | ||
const elementId = ref.current?.id || ""; | ||
const currentValue = ref.current.innerText; | ||
if (elementId.includes("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.
This string check seems fragile and prone to break in the future - is the best way to decide what this element is?
} else if (elementId.includes("textarea")) { | ||
// text | ||
// eslint-disable-next-line no-console | ||
console.log("handling blur for text"); |
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.
Once this code is ready for the PR the console log should be removed or commented out.
if (!Array.isArray(trans)) { | ||
trans = [trans]; | ||
} | ||
console.log("Normalized trans array:", trans); |
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.
Comment out or remove
|
||
// Inplace add a reference to the transcription object for all captions | ||
allTranscriptionData.forEach((captionList, listIndex) =>{ | ||
console.log("Fetched allTranscriptionData:", allTranscriptionData); |
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.
Comment out or remove
const { watch } = yield select(); | ||
|
||
console.log("Entering saveCaption with payload:", { caption, text, begin, end }); |
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.
Comment out or remove
} catch (error) { | ||
// eslint-disable-next-line no-console | ||
console.error("Error during save:", error.message); | ||
alert('Please enter a valid time format (HH:MM:SS, MM:SS, or HH:MM:SS.SSS)'); // eslint-disable-line no-alert |
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.
Please use a better alert library (ClassTranscribe already has a couple included) than the js builtin 'alert'
Is this is ready for merging in staging?e.g. Has it been sufficiently tested? Can you show me a demo? (Does it need other merges or backend changes first?) |
Allows editing of starting and ending timestamps for captions and descriptions.