-
Notifications
You must be signed in to change notification settings - Fork 0
feat: dynamic tile creation #9
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
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.
Pull request overview
This PR adds dynamic tile creation functionality to the MosaicDashboard component, allowing users to add new panels on-the-fly in horizontal (right) or vertical (below) directions. The feature includes interactive buttons in each tile's toolbar that open a dropdown for selecting which panel to add.
Key changes:
- Added Controls component with buttons to dynamically add tiles in different directions
- Implemented dropdown UI for selecting which tile type to add
- Fixed typo in TODO comment ("paramaterize" → "parameterize")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {ALL_TILES.filter((t) => t !== id).map((t) => ( | ||
| <option key={t} value={t}> | ||
| {t} | ||
| </option> | ||
| ))} |
Copilot
AI
Dec 16, 2025
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.
The tile names displayed in the dropdown and button labels are camelCase identifiers (e.g., 'mapView', 'rosMonitor') which are not user-friendly. Consider creating a mapping to display human-readable names (e.g., 'Map View', 'ROS Monitor') to improve the user experience.
| <button | ||
| className="tile-btn" | ||
| title="Add tile to the right" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setPendingAdd({ pathKey, path, direction: 'row' }); | ||
| }} | ||
| > | ||
| ➕ (Right) | ||
| </button> | ||
|
|
||
| <button | ||
| className="tile-btn" | ||
| title="Add tile below" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setPendingAdd({ pathKey, path, direction: 'column' }); | ||
| }} | ||
| > | ||
| ➕ (Below) | ||
| </button> |
Copilot
AI
Dec 16, 2025
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.
The buttons lack proper aria-label attributes for screen readers. While the title attribute is present, it only provides tooltip text on hover and is not accessible to assistive technologies. Add aria-label attributes to ensure screen reader users can understand the purpose of these buttons.
| <select | ||
| className="tile-select" | ||
| onClick={(e) => e.stopPropagation()} | ||
| onChange={(e) => { | ||
| const value = e.target.value as MosaicKey; | ||
| splitAndAdd(pendingAdd!.direction, value); | ||
| }} | ||
| defaultValue="" | ||
| > |
Copilot
AI
Dec 16, 2025
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.
The select dropdown appears immediately when either button is clicked, but lacks an accessible label. Add an aria-label attribute to the select element to describe its purpose for screen reader users, such as "Select tile to add".
| .tile-btn { | ||
| background: transparent; | ||
| border: 1px solid #444; | ||
| color: #2d2d2d; |
Copilot
AI
Dec 16, 2025
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.
The button text color is set to a very dark color (#2d2d2d) on a dark background (#2d2d2d for the title bar), making the buttons nearly invisible to users. The color should be changed to a lighter color (such as #f1f1f1) to provide sufficient contrast and ensure the buttons are visible and accessible.
| const Controls: React.FC<{ id: MosaicKey; path: MosaicPath }> = ({ id, path }) => { | ||
| const { mosaicActions } = useContext(MosaicContext); | ||
| const pathKey = JSON.stringify(path); | ||
| const showDropdown = pendingAdd?.pathKey === pathKey; | ||
|
|
||
| const splitAndAdd = (direction: 'row' | 'column', newTile: MosaicKey) => { | ||
| const splitNode: MosaicNode<MosaicKey> = { | ||
| direction, | ||
| first: id, // keep current tile | ||
| second: newTile, | ||
| splitPercentage: 60, | ||
| }; | ||
|
|
||
| mosaicActions.replaceWith(path, splitNode); | ||
| setPendingAdd(null); | ||
| }; | ||
|
|
||
| return ( | ||
| <div style={{ display: 'flex', gap: 6, alignItems: 'center' }}> | ||
| <button | ||
| className="tile-btn" | ||
| title="Add tile to the right" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setPendingAdd({ pathKey, path, direction: 'row' }); | ||
| }} | ||
| > | ||
| ➕ (Right) | ||
| </button> | ||
|
|
||
| <button | ||
| className="tile-btn" | ||
| title="Add tile below" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setPendingAdd({ pathKey, path, direction: 'column' }); | ||
| }} | ||
| > | ||
| ➕ (Below) | ||
| </button> | ||
|
|
||
| {showDropdown ? ( | ||
| <select | ||
| className="tile-select" | ||
| onClick={(e) => e.stopPropagation()} | ||
| onChange={(e) => { | ||
| const value = e.target.value as MosaicKey; | ||
| splitAndAdd(pendingAdd!.direction, value); | ||
| }} | ||
| defaultValue="" | ||
| > | ||
| <option value="" disabled> | ||
| Pick tile… | ||
| </option> | ||
| {ALL_TILES.filter((t) => t !== id).map((t) => ( | ||
| <option key={t} value={t}> | ||
| {t} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| ) : null} | ||
| </div> | ||
| ); | ||
| }; |
Copilot
AI
Dec 16, 2025
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.
The Controls component is defined inside the render function of MosaicDashboard. This will cause the Controls component to be recreated on every render, which can lead to unnecessary re-renders and potential performance issues. The Controls component should be moved outside of the MosaicDashboard component or memoized to prevent this issue.
| const ALL_TILES = useMemo<MosaicKey[]>( | ||
| () => [ | ||
| 'mapView', | ||
| 'rosMonitor', | ||
| 'networkHealthMonitor', | ||
| 'orientationDisplay', | ||
| 'videoControls', | ||
| 'waypointList', | ||
| 'gasSensor', | ||
| 'goalSetter', | ||
| ], | ||
| [] | ||
| ); |
Copilot
AI
Dec 16, 2025
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.
The ALL_TILES constant is wrapped in useMemo with an empty dependency array, but since it contains a static array of string literals that never changes, useMemo adds unnecessary overhead without any benefit. Consider removing useMemo and declaring ALL_TILES as a const array outside the component, or at minimum as a regular const inside the component.
| onChange={(e) => { | ||
| const value = e.target.value as MosaicKey; | ||
| splitAndAdd(pendingAdd!.direction, value); | ||
| }} |
Copilot
AI
Dec 16, 2025
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.
The onChange handler directly casts the value without validation and uses the non-null assertion operator (pendingAdd!). If pendingAdd becomes null between the time showDropdown is evaluated and onChange is triggered, this could cause a runtime error. Consider adding a null check before calling splitAndAdd.
| {showDropdown ? ( | ||
| <select | ||
| className="tile-select" | ||
| onClick={(e) => e.stopPropagation()} | ||
| onChange={(e) => { | ||
| const value = e.target.value as MosaicKey; | ||
| splitAndAdd(pendingAdd!.direction, value); | ||
| }} | ||
| defaultValue="" | ||
| > | ||
| <option value="" disabled> | ||
| Pick tile… | ||
| </option> | ||
| {ALL_TILES.filter((t) => t !== id).map((t) => ( | ||
| <option key={t} value={t}> | ||
| {t} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| ) : null} |
Copilot
AI
Dec 16, 2025
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.
The dropdown is only closed when a tile is selected via splitAndAdd. If a user clicks elsewhere on the page, the dropdown remains open and shows for that specific tile. Consider adding an onBlur handler or a click-outside handler to close the dropdown when the user clicks elsewhere, improving the user experience.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@ConnorNeed I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
5be2ce2 to
c26b93b
Compare
c26b93b to
b92bd54
Compare
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const value = e.target.value as MosaicKey; | ||
| // Validate pendingAdd state and selected value before proceeding | ||
| if (pendingAdd && value) { | ||
| splitAndAdd(pendingAdd.direction, value); |
Copilot
AI
Dec 16, 2025
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.
The onChange handler doesn't reset the select element's value after selection. This means after selecting a tile, the dropdown will show the last selected tile instead of the placeholder "Pick tile…". Consider adding e.target.value = '' after calling splitAndAdd to reset the selection, or use a controlled component with value state.
| splitAndAdd(pendingAdd.direction, value); | |
| splitAndAdd(pendingAdd.direction, value); | |
| e.target.value = ''; |
|
Code still has some bugs. Closing for now |
Nothing benefits more from me procrastinating from exams than this webUI.



