feat: add include description functionality to ContainerComponent#2808
feat: add include description functionality to ContainerComponent#2808
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to toggle the description field visibility in the ContainerComponent through a checkbox control. The feature allows users to optionally include or exclude the description field, automatically clearing the description when unchecked.
Key Changes:
- Added
includeDescriptionstate to manage description field visibility - Implemented checkbox control to toggle description inclusion
- Adjusted column layout from 8/4 to 6/3/3 split for better accommodation of the new checkbox
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Chemotion.dev.5.mp4 |
TasnimMehzabin
force-pushed
the
2743-add-checkbox-to-enable-analysis-description
branch
2 times, most recently
from
e10810a to
c7ebed8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Introduced a checkbox to toggle the inclusion of a description in the container. - Updated state management to handle the description visibility based on the checkbox state. - Adjusted layout for better responsiveness and user experience.
…ainerComponent - Simplified state updates when toggling the description checkbox. - Ensured proper propagation of container updates via `onChange` callback. - Enhanced readability and maintainability of conditional logic.
…onent - Introduced `includeDescriptionIdCounter` to ensure unique IDs for the checkbox. - Updated logic to generate IDs dynamically based on index or counter suffix.
TasnimMehzabin
force-pushed
the
2743-add-checkbox-to-enable-analysis-description
branch
from
c7ebed8 to
259b796
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const updatedContainer = { ...container, description: '' }; | ||
| this.setState( | ||
| { includeDescription, container: updatedContainer }, | ||
| () => onChange(updatedContainer) |
There was a problem hiding this comment.
The implementation of handleIncludeDescriptionChange creates a new container object using spread syntax ({ ...container, description: '' }), which is inconsistent with the existing pattern in the codebase. The existing handleInputChange method (lines 70-103) mutates the state container directly before passing it to onChange.
While creating a new object is generally a better practice in React, mixing both patterns in the same component could lead to confusion. Consider either:
- Following the existing pattern by mutating
container.descriptiondirectly, or - Refactoring the entire component to use immutable updates consistently.
For consistency with the current codebase, the mutation approach would be: container.description = ''; this.setState({ includeDescription, container }, () => onChange(container));
| const updatedContainer = { ...container, description: '' }; | |
| this.setState( | |
| { includeDescription, container: updatedContainer }, | |
| () => onChange(updatedContainer) | |
| container.description = ''; | |
| this.setState( | |
| { includeDescription, container }, | |
| () => onChange(container) |
| @@ -24,8 +26,11 @@ export default class ContainerComponent extends Component { | |||
| const textTemplate = TextTemplateStore.getState()[templateType] || Map(); | |||
| this.state = { | |||
| container, | |||
| textTemplate: textTemplate && textTemplate.toJS() | |||
| textTemplate: textTemplate && textTemplate.toJS(), | |||
| includeDescription: !!(container?.description) | |||
| }; | |||
| includeDescriptionIdCounter += 1; | |||
| this.includeDescriptionIdSuffix = includeDescriptionIdCounter; | |||
There was a problem hiding this comment.
The global counter includeDescriptionIdCounter will persist across component instances and will not reset when components are unmounted. This could lead to unnecessarily large ID values over time. Additionally, using a module-level variable for component-specific logic is not a typical React pattern and could cause issues in testing environments where modules are cached.
Consider using React.useId() hook (if migrating to functional components) or generating IDs based on more stable identifiers like the container ID if available. Alternatively, if the index prop is always provided (as shown in usage examples), you could rely solely on the index without the counter fallback.
| this.setState({ | ||
| container, | ||
| includeDescription: !!(container?.description) | ||
| }); |
There was a problem hiding this comment.
When componentDidUpdate is triggered by a container prop change, the includeDescription state is reset based on whether the container has a description. However, this logic doesn't account for user intent. If a user unchecks the "Include description" checkbox (clearing the description), then the parent component updates and passes the same container back, the checkbox will remain unchecked since there's no description. But if a different container with a non-empty description is passed in, the checkbox will be automatically checked, which is the correct behavior.
However, there's an edge case: if the user checks the checkbox (to enable the description field) but hasn't yet typed anything, then the component updates with a new container, the checkbox state will be determined by the new container's description content. This might be acceptable behavior, but it's worth verifying this matches the intended UX.
Closes #2743 and #2767