make Internalize step mobile friendly #236
make Internalize step mobile friendly #236abigaillew4 wants to merge 20 commits intosillsdev:developfrom
Conversation
Add to Research
| variant="outlined" | ||
| onClick={props.handleConfigureGeneral} | ||
| sx = {{color: 'black', borderBlockColor: 'black'}}> | ||
| Configure General |
There was a problem hiding this comment.
Note: Strings hardcoded here and need to be localized
| }} | ||
| /> | ||
| } | ||
| label="Show resources for all passages" |
There was a problem hiding this comment.
Note: Strings hardcoded here and need to be localized
There was a problem hiding this comment.
Pull request overview
This PR improves the Internalize step’s mobile UX by introducing mobile-specific artifact/resource UI, adding responsive behavior to resource-finding/data-table views, and extending shared UI components to be more configurable for smaller screens.
Changes:
- Added mobile-focused Internalization artifact list UI (cards, settings dialog) and wired it into Passage detail on mobile.
- Enhanced shared UI components (dialogs, markdown view/edit, drag-and-drop list, media player, buttons) for responsiveness and customization.
- Improved responsiveness in resource-finding (Aquifer/BibleBrain tabs) and passage data table; bumped
@fingerprintjs/fingerprintjs.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/renderer/src/routes/PassageDetail.tsx | Switches artifacts UI to a new mobile component when on mobile. |
| src/renderer/src/hoc/VertListDnd.tsx | Adds optional vertical-only drag behavior + spacing/padding customization for mobile lists. |
| src/renderer/src/hoc/BigDialog.tsx | Adds customization for title, close buttons, outlines, and mobile scrollbar/scroll behavior. |
| src/renderer/src/control/MarkDownView.tsx | Adds overflow-wrapping option + a compact markdown renderer for mobile dialogs. |
| src/renderer/src/control/MarkDownEdit.tsx | Adds optional wrapped preview overflow behavior; moves help icon into preview header. |
| src/renderer/src/control/AltButton.tsx | Adds dark/elevated styling options for consistent mobile buttons. |
| src/renderer/src/components/Uploader.tsx | Allows configuring upload dialog breakpoint passed down to MediaUpload. |
| src/renderer/src/components/PassageDetail/Internalization/mobile components/TextResourceCard.tsx | New mobile card for non-audio resources (markdown/links/files). |
| src/renderer/src/components/PassageDetail/Internalization/mobile components/SettingsDialog.tsx | New mobile settings dialog for internalization resource toggles/actions. |
| src/renderer/src/components/PassageDetail/Internalization/mobile components/AudioResourceCard.tsx | New mobile card for audio resources embedding LimitedMediaPlayer. |
| src/renderer/src/components/PassageDetail/Internalization/index.ts | Exports the new AudioResourceCard component. |
| src/renderer/src/components/PassageDetail/Internalization/ResourceData.tsx | Plumbs wrapOverflow option into markdown editing preview. |
| src/renderer/src/components/PassageDetail/Internalization/PassageDetailsArtifactsMobile.tsx | Large new mobile artifacts/resources experience for Internalization step. |
| src/renderer/src/components/PassageDetail/Internalization/FindTabs.tsx | Makes find-resource tabs mobile-friendly (dropdown + launch + scroll handling). |
| src/renderer/src/components/PassageDetail/Internalization/FindAquifer.tsx | Improves horizontal scrolling support around the grid on small screens. |
| src/renderer/src/components/PassageDetail/Internalization/AddResource.tsx | Adds customization props for the “Add Resource” button; simplifies menu. |
| src/renderer/src/components/PassageDataTable.tsx | Adjusts controls layout/widths for small screens. |
| src/renderer/src/components/MediaUpload.tsx | Adds bp prop to control dialog breakpoint. |
| src/renderer/src/components/LimitedMediaPlayer.tsx | Adds optional containerless controls and configurable play button size. |
| package.json | Bumps @fingerprintjs/fingerprintjs to ^5.1.0. |
Comments suppressed due to low confidence (1)
src/renderer/src/control/MarkDownView.tsx:20
- This only handles clicks when the direct event target is the
<a>element; if the link contains nested elements (e.g.,<a><span>…</span></a>),event.targetwon’t beaand the link won’t open externally. Useevent.target.closest('a')(with proper typing) or render markdown links with a customacomponent viareact-markdownso link behavior is handled reliably without a global document click listener.
const handleClick = (event: any) => {
if (event.target.tagName.toLowerCase() === 'a') {
event.preventDefault();
ipc?.openExternal(event.target.href);
}
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -9,6 +20,22 @@ export const AltButton = ({ children, ...rest }: ButtonProps) => ( | |||
| overflow: 'hidden', | |||
| whiteSpace: 'nowrap', | |||
| justifyContent: 'flex-start', | |||
| ...(dark | |||
| ? { | |||
| color: 'common.black', | |||
| borderColor: 'common.black', | |||
| '&:hover': { | |||
| borderColor: 'common.black', | |||
| backgroundColor: 'rgba(0,0,0,0.04)', | |||
| }, | |||
| } | |||
| : {}), | |||
| ...(elevated | |||
| ? { | |||
| boxShadow: '0 1px 3px rgba(0,0,0,0.22)', | |||
| } | |||
| : {}), | |||
| ...sx, | |||
| }} | |||
| {...rest} | |||
| > | |||
There was a problem hiding this comment.
sx is SxProps, which can be an array or a function; spreading it (...sx) will misbehave (arrays become numeric keys, functions won’t apply). Build a base style object and pass sx as an array merge (e.g., sx={[baseSx, sx]}) or conditionally merge only when sx is a plain object.
| ...(bp === BigDialogBp.mobile && mobileThickScrollbar | ||
| ? { | ||
| '& .MuiDialogContent-root': { | ||
| scrollbarColor: '#666 #d0d0d0', | ||
| scrollbarWidth: 'auto', | ||
| '&::-webkit-scrollbar': { | ||
| width: '14px', | ||
| height: '14px', | ||
| }, | ||
| '&::-webkit-scrollbar-thumb': { | ||
| backgroundColor: '#666', | ||
| borderRadius: '8px', | ||
| }, | ||
| '&::-webkit-scrollbar-track': { | ||
| backgroundColor: '#d0d0d0', | ||
| }, | ||
| }, | ||
| } | ||
| : {}), | ||
| ...(bp === BigDialogBp.mobile && mobileNoHorizontalScroll | ||
| ? { | ||
| '& .MuiDialogContent-root': { | ||
| overflowX: 'hidden', | ||
| }, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
When both mobileThickScrollbar and mobileNoHorizontalScroll are true, the second spread overwrites & .MuiDialogContent-root entirely, dropping the scrollbar styling. Merge these settings into a single & .MuiDialogContent-root object (or compose them without overwriting) so both behaviors apply together.
| ...(bp === BigDialogBp.mobile && mobileThickScrollbar | |
| ? { | |
| '& .MuiDialogContent-root': { | |
| scrollbarColor: '#666 #d0d0d0', | |
| scrollbarWidth: 'auto', | |
| '&::-webkit-scrollbar': { | |
| width: '14px', | |
| height: '14px', | |
| }, | |
| '&::-webkit-scrollbar-thumb': { | |
| backgroundColor: '#666', | |
| borderRadius: '8px', | |
| }, | |
| '&::-webkit-scrollbar-track': { | |
| backgroundColor: '#d0d0d0', | |
| }, | |
| }, | |
| } | |
| : {}), | |
| ...(bp === BigDialogBp.mobile && mobileNoHorizontalScroll | |
| ? { | |
| '& .MuiDialogContent-root': { | |
| overflowX: 'hidden', | |
| }, | |
| } | |
| : {}), | |
| ...(bp === BigDialogBp.mobile && (mobileThickScrollbar || mobileNoHorizontalScroll) | |
| ? { | |
| '& .MuiDialogContent-root': { | |
| ...(mobileThickScrollbar && { | |
| scrollbarColor: '#666 #d0d0d0', | |
| scrollbarWidth: 'auto', | |
| '&::-webkit-scrollbar': { | |
| width: '14px', | |
| height: '14px', | |
| }, | |
| '&::-webkit-scrollbar-thumb': { | |
| backgroundColor: '#666', | |
| borderRadius: '8px', | |
| }, | |
| '&::-webkit-scrollbar-track': { | |
| backgroundColor: '#d0d0d0', | |
| }, | |
| }), | |
| ...(mobileNoHorizontalScroll && { | |
| overflowX: 'hidden', | |
| }), | |
| }, | |
| } | |
| : {}), |
| label="Show resources for all passages" | ||
| /> | ||
| )} | ||
| {props.showConfigureGeneral && ( | ||
| <Button | ||
| variant="outlined" | ||
| onClick={props.handleConfigureGeneral} | ||
| sx = {{color: 'black', borderBlockColor: 'black'}}> | ||
| Configure General | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
This introduces hard-coded UI strings and a nonstandard style prop (borderBlockColor) for an outlined button. Use existing localized strings (consistent with the rest of the Internalization UI) and prefer sx={{ color: 'black', borderColor: 'black' }} (also remove the spaced sx = formatting which is inconsistent with surrounding code).
| const cancelled = useRef(false); | ||
| const [displayId, setDisplayId] = useState(''); | ||
| const [link, setLink] = useState<string>(); | ||
| const [markDown, setMarkDoan] = useState(''); |
There was a problem hiding this comment.
setMarkDoan looks like a typo and makes the code harder to follow/grep. Rename it to setMarkDown to match the markDown state variable.
| const [markDown, setMarkDoan] = useState(''); | |
| const [markDown, setMarkDown] = useState(''); |
| setMarkDownTitle( | ||
| rowData.find((r) => r.id === id)?.artifactName || t.textResource | ||
| ); | ||
| setMarkDoan( |
There was a problem hiding this comment.
setMarkDoan looks like a typo and makes the code harder to follow/grep. Rename it to setMarkDown to match the markDown state variable.
| setMarkDoan( | |
| setMarkDown( |
| const getSectionType = () => { | ||
| const level = section.attributes?.level; | ||
| if (level === SheetLevel.Book) return 'BOOK'; | ||
| if (level === SheetLevel.Movement) return 'MOVE'; | ||
| return undefined; | ||
| }; | ||
|
|
||
| const sectDesc = useMemo( | ||
| () => | ||
| getSectionType() === PassageTypeEnum.BOOK | ||
| ? t.bookResource | ||
| : getSectionType() === PassageTypeEnum.MOVEMENT | ||
| ? t.movementResource | ||
| : getOrganizedBy(true), |
There was a problem hiding this comment.
getSectionType() returns 'MOVE' for SheetLevel.Movement, but sectDesc compares it to PassageTypeEnum.MOVEMENT. This makes the movement branch unreachable and will fall back to getOrganizedBy(true) instead of t.movementResource. Return the correct enum value/string (e.g., 'MOVEMENT' or PassageTypeEnum.MOVEMENT) to match the comparisons.
|
|
||
| useEffect(() => { | ||
| if (!projResPassageVisible && !projResWizVisible && projMediaRef.current) | ||
| setProjResSetup(projResSetup.filter((m) => m !== projMediaRef.current)); |
There was a problem hiding this comment.
This effect reads projResSetup but doesn’t include it in the dependency array, so it can apply a stale filter and drop the wrong entries. Use a functional state update (setProjResSetup(prev => prev.filter(...))) to avoid stale closures, or include projResSetup in deps.
| setProjResSetup(projResSetup.filter((m) => m !== projMediaRef.current)); | |
| setProjResSetup((prev) => | |
| prev.filter((m) => m !== projMediaRef.current) | |
| ); |
| onClick={() => handleFindVisible(true)} | ||
| sx={{ fontSize: (theme) => theme.typography.h6.fontSize }} | ||
| > | ||
| <Badge>{t.research}</Badge> |
There was a problem hiding this comment.
Badge is being used without badgeContent, so it won’t render as intended (and may not render any badge at all). If you just want styled text, remove Badge and render {t.research} directly; if you want a badge, pass badgeContent and wrap the label component correctly.
| <Badge>{t.research}</Badge> | |
| {t.research} |
| if (transform.startsWith('translate3d(')) { | ||
| // translate3d(xpx, ypx, zpx) -> lock x to 0px | ||
| style.transform = transform.replace( | ||
| /translate3d\([^,]+,\s*([^,]+),\s*([^\)]+)\)/, | ||
| 'translate3d(0px, $1, $2)' | ||
| ); | ||
| return style; | ||
| } | ||
| if (transform.startsWith('translate(')) { | ||
| // translate(xpx, ypx) -> lock x to 0px | ||
| style.transform = transform.replace( | ||
| /translate\([^,]+,\s*([^\)]+)\)/, | ||
| 'translate(0px, $1)' | ||
| ); | ||
| return style; | ||
| } | ||
| return style; |
There was a problem hiding this comment.
This mutates the style object coming from provided.draggableProps.style. Mutating library-provided objects can cause hard-to-debug side effects. Prefer returning a new object (e.g., copy style first and override transform) so the original object remains untouched.
| if (transform.startsWith('translate3d(')) { | |
| // translate3d(xpx, ypx, zpx) -> lock x to 0px | |
| style.transform = transform.replace( | |
| /translate3d\([^,]+,\s*([^,]+),\s*([^\)]+)\)/, | |
| 'translate3d(0px, $1, $2)' | |
| ); | |
| return style; | |
| } | |
| if (transform.startsWith('translate(')) { | |
| // translate(xpx, ypx) -> lock x to 0px | |
| style.transform = transform.replace( | |
| /translate\([^,]+,\s*([^\)]+)\)/, | |
| 'translate(0px, $1)' | |
| ); | |
| return style; | |
| } | |
| return style; | |
| let newTransform = transform; | |
| if (transform.startsWith('translate3d(')) { | |
| // translate3d(xpx, ypx, zpx) -> lock x to 0px | |
| newTransform = transform.replace( | |
| /translate3d\([^,]+,\s*([^,]+),\s*([^\)]+)\)/, | |
| 'translate3d(0px, $1, $2)' | |
| ); | |
| } else if (transform.startsWith('translate(')) { | |
| // translate(xpx, ypx) -> lock x to 0px | |
| newTransform = transform.replace( | |
| /translate\([^,]+,\s*([^\)]+)\)/, | |
| 'translate(0px, $1)' | |
| ); | |
| } | |
| if (newTransform === transform) { | |
| return style; | |
| } | |
| return { | |
| ...style, | |
| transform: newTransform, | |
| }; |
This pull request introduces several UI and usability improvements, particularly focused on responsiveness and customization for media and data table components. It also adds new customization options for buttons and dialogs, and includes some code cleanup. The most notable changes are grouped below:
Media Player and Upload Enhancements:
LimitedMediaPlayer.tsx: AddedplayButtonSizeandnoContainerprops to allow customizing the play/pause button size and to optionally render controls without the container chip. Refactored the controls rendering logic for more flexibility and better styling, especially for different layouts. [1] [2] [3] [4] [5] [6]MediaUpload.tsx: Added abpprop to allow customizing the dialog breakpoint size, defaulting to small if not provided. [1] [2] [3]Responsive Design Improvements:
PassageDataTable.tsx: Improved responsiveness for small screens by adjusting layout, spacing, and element widths usinguseMediaQueryanduseTheme. This ensures better usability on mobile devices. [1] [2] [3] [4] [5] [6]FindAquifer.tsx: Enhanced horizontal scrolling and layout handling for wide content, ensuring the data grid and controls remain accessible on smaller screens. [1] [2] [3] [4]Button and Menu Customization:
AddResource.tsx: Simplified the component by removing unused code and props, and added new props (buttonDark,buttonElevated,buttonSx) to allow customizing the appearance of the "Add Resource" button. [1] [2] [3]Resource Finder Tabs and Structure:
FindTabs.tsx: Refactored and prepared for additional tabbed resource finders, including responsive design support and setup for new tabs like BibleBrain and FaithBridge, with indices managed for clarity. [1] [2] [3]Dependency Updates:
package.json: Upgraded@fingerprintjs/fingerprintjsfrom version 5.0.1 to 5.1.0.