Revise Settings Dialog with New UX Design #1354
Conversation
…ar, removed uses of user
… component, attempt to fix scrolling
…vise-settings-dialog
There was a problem hiding this comment.
Lots of good work here! I have several questions to make sure I'm following everything properly. Also, I'm not at all familiar with what we're doing with Tailwind styling, so I can't comment on anything there. If that needs a closer look, please ask someone who is familiar with it.
Reviewed 39 of 39 files at r1, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @jolierabideau)
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx line 20 at r1 (raw file):
}; export type ProjectOptions = { projectId: string; projectName: string };
There aren't really any "options" here. I would like something like ProjectInfo would make more sense, but it's not a huge deal.
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx line 45 at r1 (raw file):
/** Placeholder for the button */ buttonPlaceholder: string;
What's the point of a placeholder here? I would think we'd either put a button in or not have a placeholder.
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx line 99 at r1 (raw file):
{ 'tw-bg-white tw-text-gray-900 tw-shadow-sm': getIsActive(label) }, )} onClick={() => handleSelectItem(label, undefined)}
You could change handleSelectItem(label, undefined) to just handleSelectItem(label) here since the second argument is optional.
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar-content-search.component.tsx line 12 at r1 (raw file):
/** Text direction ltr or rtl */ direction?: 'ltr' | 'rtl';
Why do we need to explicitly call this out here as an input parameter? I would have expected this to be derived from either the current UI locale or the language(s) represented in the search string.
src/renderer/services/web-view.service-host.ts line 1439 at r1 (raw file):
type: 'float', position: 'center', floatSize: { height: 575, width: 950 },
Where are we getting the size constants?
lib/platform-bible-react/src/components/shadcn-ui/sheet.tsx line 0 at r1 (raw file):
I don't do a lot of UI programming, but I'm not sure what a Sheet is intended to be along with all the things we're providing for it. I don't see any documentation below, so I'm guessing this is something like a spreadsheet. But then I start wondering what the different is between a sheet, a table, and a grid unless sheets have formulas which I don't see below. I think some guidance docs would be nice to add for people who want to use sheets.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 21 at r1 (raw file):
const SIDEBAR_COOKIE_NAME = 'sidebar:state'; const SIDEBAR_COOKIE_MAX_AGE = 60 * 60 * 24 * 7;
What happens a week after the max age is set? Does some setting disappear or reset to a default value? I think we've just been saving things directly to localStorage instead of using cookies. @tjcouch-sil do you see concerns with mixing in cookies with our UI?
Where does the cookie get read? I only see us writing it, not reading it.
If we do need cookies for some reason, I guess I would expect them to never expire (or set an expiration to something like 2^31 which would have the same effect).
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 98 at r1 (raw file):
React.useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { if (event.key === SIDEBAR_KEYBOARD_SHORTCUT && (event.metaKey || event.ctrlKey)) {
Was this specified by UX? Just thinking about if we are trying to create equivalent shortcut keys for PT9 and/or if we should be writing down shortcut keys anywhere that we are using for different things to avoid collisions.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 152 at r1 (raw file):
}, ); SidebarProvider.displayName = 'SidebarProvider';
Does this actually get displayed somewhere? If not, what's the point of it? The same goes for cases below where you set the displayName property.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 279 at r1 (raw file):
> <PanelLeft /> <span className="tw-sr-only">Toggle Sidebar</span>
Does this need to be localized? I don't see where it gets drawn.
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 3 at r1 (raw file):
import * as React from 'react'; const MOBILE_BREAKPOINT = 768;
Where did this constant come from?
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 5 at r1 (raw file):
const MOBILE_BREAKPOINT = 768; export default function useIsMobile() {
It would be good to add a return type and a JSDOC-style description to this function. I'm not sure what "mobile" is intended to mean in this context. In the absence of context I would assume it means "running on a smartphone" which I doubt is the intent since we're not targeting smartphone OSes.
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 9 at r1 (raw file):
React.useEffect(() => { const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`);
Just curious, what is mql in this context? I think I've only ever seen "QL" at the end of an abbreviation when it stands for "query language", but I'm pretty sure that's not the intent here.
src/renderer/components/settings-tabs/settings-tab.component.tsx line 185 at r1 (raw file):
); if (!settingsContributions) return <div className="settings-tab">No Settings</div>;
This looks like a string that should be localized.
src/shared/services/project-settings.service.ts line 48 at r1 (raw file):
const { includeProjectInterfaces, excludeProjectInterfaces } = property; if (includeProjectInterfaces || excludeProjectInterfaces) {
Good catch here
lib/platform-bible-react/src/components/basics/search-bar.component.tsx line 6 at r1 (raw file):
export type SearchBarProps = { className?: string;
All properties should probably have some JSDOC description
There was a problem hiding this comment.
Thanks for the review!
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @lyonsil and @tj)
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar-content-search.component.tsx line 12 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Why do we need to explicitly call this out here as an input parameter? I would have expected this to be derived from either the current UI locale or the language(s) represented in the search string.
I copied the TabNavigationContentSearch component to start this one, previously I added direction to the TabNavigationContentSearch component because I used it directly in the preview, and there we pass the selected direction into the components. Since this component isn't in the preview, I can remove it.
lib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx line 45 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
What's the point of a placeholder here? I would think we'd either put a button in or not have a placeholder.
It should be buttonPlaceholderText, I am passing a localized string into this prop.
lib/platform-bible-react/src/components/basics/search-bar.component.tsx line 6 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
All properties should probably have some JSDOC description
Done.
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 3 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Where did this constant come from?
When you use the npx script to add a Shadcn component, it adds the other component/hook dependencies that it needs. This was one of them. It is used in shadcn-ui/sidebar.tsx. This constant came from Shadcn.
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 5 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
It would be good to add a return type and a JSDOC-style description to this function. I'm not sure what "mobile" is intended to mean in this context. In the absence of context I would assume it means "running on a smartphone" which I doubt is the intent since we're not targeting smartphone OSes.
You are right, I don't see us using this. It is only used in shadcn-ui/sidebar.tsx, and I didn't export it so it can't be accessed outside of PBR. I can remove the use of this hook in shadcn-ui/sidebar.tsx and delete the file?
lib/platform-bible-react/src/components/hooks/use-mobile.tsx line 9 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Just curious, what is
mqlin this context? I think I've only ever seen "QL" at the end of an abbreviation when it stands for "query language", but I'm pretty sure that's not the intent here.
Looks like it stands for MediaQueryList. https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList
lib/platform-bible-react/src/components/shadcn-ui/sheet.tsx line at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
I don't do a lot of UI programming, but I'm not sure what a
Sheetis intended to be along with all the things we're providing for it. I don't see any documentation below, so I'm guessing this is something like a spreadsheet. But then I start wondering what the different is between a sheet, a table, and a grid unless sheets have formulas which I don't see below. I think some guidance docs would be nice to add for people who want to use sheets.
This is another of the components that the script added as a dependency for Sidebar. Shadcn Sheet extends the Radix Dialog component. The Sidebar actually just renders a Sheet. I also did not export this component so it cannot be accessed outside of PBR.
https://ui.shadcn.com/docs/components/sheet
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 21 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
What happens a week after the max age is set? Does some setting disappear or reset to a default value? I think we've just been saving things directly to
localStorageinstead of using cookies. @tjcouch-sil do you see concerns with mixing in cookies with our UI?Where does the cookie get read? I only see us writing it, not reading it.
If we do need cookies for some reason, I guess I would expect them to never expire (or set an expiration to something like 2^31 which would have the same effect).
I am not sure what happens with these cookies. This is more of the code that was generated from Shadcn. I can work on removing this, and seeing if that changes the functionality of the Sidebar.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 98 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Was this specified by UX? Just thinking about if we are trying to create equivalent shortcut keys for PT9 and/or if we should be writing down shortcut keys anywhere that we are using for different things to avoid collisions.
That's a good thought. I can talk with UX about this.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 152 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Does this actually get displayed somewhere? If not, what's the point of it? The same goes for cases below where you set the
displayNameproperty.
All of the Shadcn-generated components have this. From the little research I did, it appears that React sometimes does not do a good job of inferring the displayName when you use functional components, so setting it explicitly is supposed to help it appear correctly in debugging tools and error messages.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 279 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Does this need to be localized? I don't see where it gets drawn.
I would say yes, I didn't use the SidebarTrigger so I didn't notice this.
src/renderer/services/web-view.service-host.ts line 1439 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Where are we getting the size constants?
I meant to change this. I took this from UX's Figma, as I was testing I wanted to render the tab larger. I am not sure what I should change it to?
src/shared/services/project-settings.service.ts line 48 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Good catch here
That was all TJ!
… add missing localized string
There was a problem hiding this comment.
Reviewed 1 of 39 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @jolierabideau, @lyonsil, and @tj)
lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 25 at r1 (raw file):
options?: readonly T[]; /** Additional css classes to help with unique styling of the combo box */ className?: string;
At this point, we are trying to avoid breaking changes where reasonably possible and instead deprecate old things. Is it reasonably easy to keep this prop in the software but just add the following JSDoc comment to it?
/** @deprecated 3 December 2024. Renamed to {@link buttonClassName} */(Note this prop's jsdoc should only be this, not the full description, so we reduce duplication and people have to go to the new prop to see the jsdoc comments and maybe feel more incentivized to use the new prop)
lib/papi-dts/papi.d.ts line 2901 at r1 (raw file):
/** Restart the application */ 'platform.restart': () => Promise<void>; 'platform.openProjectSettings': (webViewId: string) => Promise<void>;
At this point, we are trying to avoid breaking changes where reasonably possible and instead deprecate old things. Is it reasonably easy to keep these commands in the software but just add the following JSDoc comment to them?
/** @deprecated 3 December 2024. Renamed to `platform.openSettings` */
There was a problem hiding this comment.
Reviewed 18 of 18 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jolierabideau)
lib/platform-bible-react/src/components/basics/search-bar.component.tsx line 6 at r1 (raw file):
Previously, jolierabideau wrote…
Done.
Sorry, I was talking about className since all the other properties of SearchBarProps have descriptions. It is good for the types to have docs, too.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 21 at r1 (raw file):
Previously, jolierabideau wrote…
I am not sure what happens with these cookies. This is more of the code that was generated from Shadcn. I can work on removing this, and seeing if that changes the functionality of the
Sidebar.
Thanks! I was a bit confused about introducing concepts we haven't been using for any reason.
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 152 at r1 (raw file):
Previously, jolierabideau wrote…
All of the Shadcn-generated components have this. From the little research I did, it appears that React sometimes does not do a good job of inferring the displayName when you use functional components, so setting it explicitly is supposed to help it appear correctly in debugging tools and error messages.
Thanks for digging into this!
src/renderer/services/web-view.service-host.ts line 1439 at r1 (raw file):
Previously, jolierabideau wrote…
I meant to change this. I took this from UX's Figma, as I was testing I wanted to render the tab larger. I am not sure what I should change it to?
This is probably a good question for UX.
lib/platform-bible-react/src/index.ts line 46 at r2 (raw file):
default as SettingsSidebar, type SettingsSidebarProps, type ProjectInfo as ProjectOptions,
This might have been done by VSCode. I'm assuming the intent it not to rename this type on the export.
There was a problem hiding this comment.
Reviewable status: 30 of 39 files reviewed, 5 unresolved discussions (waiting on @lyonsil)
lib/platform-bible-react/src/index.ts line 46 at r2 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
This might have been done by VSCode. I'm assuming the intent it not to rename this type on the export.
Done.
lib/platform-bible-react/src/components/basics/search-bar.component.tsx line 6 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Sorry, I was talking about
classNamesince all the other properties ofSearchBarPropshave descriptions. It is good for the types to have docs, too.
Oops! Fixed
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 98 at r1 (raw file):
Previously, jolierabideau wrote…
That's a good thought. I can talk with UX about this.
I have made a note to talk with Sebastian about this in our next components meeting!
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 279 at r1 (raw file):
Previously, jolierabideau wrote…
I would say yes, I didn't use the
SidebarTriggerso I didn't notice this.
This component uses the sr-only className, which means this is hidden visually without hiding it from screen readers. I am not sure how best to localize this string, besides adding a prop that we pass into the Shadcn sidebar? I am curious if I could bring this up with Sebastian as well since there are a few Shadcn components that have text like this, and then come up with an issue to fix all of them with what we decide?
src/renderer/services/web-view.service-host.ts line 1439 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
This is probably a good question for UX.
Posted in the UX channel, so hopefully I will have an answer soon
There was a problem hiding this comment.
Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 279 at r1 (raw file):
Previously, jolierabideau wrote…
This component uses the
sr-onlyclassName, which means this is hidden visually without hiding it from screen readers. I am not sure how best to localize this string, besides adding a prop that we pass into the Shadcn sidebar? I am curious if I could bring this up with Sebastian as well since there are a few Shadcn components that have text like this, and then come up with an issue to fix all of them with what we decide?
Sure, that makes sense. Resolving this for now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)
lib/platform-bible-react/src/components/shadcn-ui/sidebar.tsx line 98 at r1 (raw file):
Previously, jolierabideau wrote…
I have made a note to talk with Sebastian about this in our next components meeting!
Commented this part out pending the discussion with Sebastian
src/renderer/services/web-view.service-host.ts line 1439 at r1 (raw file):
Previously, jolierabideau wrote…
Posted in the UX channel, so hopefully I will have an answer soon
It seems they do not have an answer yet on which constants to use here. I rounded to the nearest hundred, and peeked at the widths and heights of other floating tabs. The configure checks and inventories web views render at w: 700 and h: 800, so it doesn't seem like it is too much larger than others. Do you think we could go ahead and merge?
…vise-settings-dialog
There was a problem hiding this comment.
Thanks for the adjustments! Sorry I wasn't super clear on how to deprecate before. It's a new thing I just started doing a bit ago, and I haven't yet written about it in the style guide. I added some clarifications :)
Reviewed 2 of 18 files at r2.
Reviewable status: 33 of 39 files reviewed, 5 unresolved discussions (waiting on @jolierabideau and @lyonsil)
src/renderer/services/web-view.service-host.ts line 1424 at r4 (raw file):
}; async function openSettingsTab(webViewId: WebViewId): Promise<Layout | undefined> {
Currently, platform.openProjectSettings and platform.openUserSettings are silently broken. because they are no longer used.
To deprecate a feature, it still needs to be used so the feature still works for those who haven't yet upgraded to the newer thing. So in this case, you'd want to keep those two commands in the commandHandlers object but just have them both run openSettingsTab just like the platform.openSettings command currently does.
lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 97 at r4 (raw file):
aria-expanded={open} id={id} className={cn('tw-w-[200px] tw-justify-between', buttonClassName)}
Currently, className is silently broken. because it is no longer used.
To deprecate a feature, it still needs to be used so the feature still works for those who haven't yet upgraded to the newer thing. So in this case, you'd want to use the className prop like you used to use it while also making buttonClassName work. I'd suggest something like the following:
className={cn('tw-w-[200px] tw-justify-between', buttonClassName ?? className)}lib/papi-dts/papi.d.ts line 2902 at r4 (raw file):
'platform.restart': () => Promise<void>; 'platform.openProjectSettings': (webViewId: string) => Promise<void>; 'platform.openUserSettings': () => Promise<void>;
Perfect job on the deprecation of the project settings command! We also need the openUserSettings command to stick around, too, though :) sorry for the unclear direction before!
There was a problem hiding this comment.
Reviewable status: 30 of 39 files reviewed, 5 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)
lib/papi-dts/papi.d.ts line 2902 at r4 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Perfect job on the deprecation of the project settings command! We also need the
openUserSettingscommand to stick around, too, though :) sorry for the unclear direction before!
Done.
lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 97 at r4 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Currently,
classNameis silently broken. because it is no longer used.To deprecate a feature, it still needs to be used so the feature still works for those who haven't yet upgraded to the newer thing. So in this case, you'd want to use the
classNameprop like you used to use it while also makingbuttonClassNamework. I'd suggest something like the following:className={cn('tw-w-[200px] tw-justify-between', buttonClassName ?? className)}
Done.
src/renderer/services/web-view.service-host.ts line 1424 at r4 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Currently,
platform.openProjectSettingsandplatform.openUserSettingsare silently broken. because they are no longer used.To deprecate a feature, it still needs to be used so the feature still works for those who haven't yet upgraded to the newer thing. So in this case, you'd want to keep those two commands in the
commandHandlersobject but just have them both runopenSettingsTabjust like theplatform.openSettingscommand currently does.
Done.
There was a problem hiding this comment.
Thanks for the fixes! Looks great :)
Reviewed 3 of 8 files at r5.
Reviewable status: 33 of 39 files reviewed, 2 unresolved discussions (waiting on @lyonsil)
lib/papi-dts/papi.d.ts line 2902 at r4 (raw file):
Previously, jolierabideau wrote…
Done.
Looks like maybe this needs to be updated with npm run build:types - the pre-commit script no longer runs this for you as of maybe last week. The build server will run it and will fail the build check if it detects papi.d.ts changes, though. I have already received failures multiple times because I forgot to run npm run build:types 😂
There was a problem hiding this comment.
Reviewable status: 27 of 39 files reviewed, 2 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)
lib/papi-dts/papi.d.ts line 2902 at r4 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Looks like maybe this needs to be updated with
npm run build:types- the pre-commit script no longer runs this for you as of maybe last week. The build server will run it and will fail the build check if it detectspapi.d.tschanges, though. I have already received failures multiple times because I forgot to runnpm run build:types😂
Yes it did! Sorry, thanks for catching that! Hopefully all is good now.
|
This PR #1354 partial satisfies UX designs to improve Settings New mockups in response to PR #1354 for clarity and improvement:
which combines
These do not block the PR. |
There was a problem hiding this comment.
Reviewed 1 of 6 files at r4, 1 of 8 files at r5, 4 of 11 files at r6, all commit messages.
Reviewable status: 32 of 39 files reviewed, all discussions resolved (waiting on @tjcouch-sil)
There was a problem hiding this comment.
Reviewed 3 of 11 files at r6.
Reviewable status: 35 of 39 files reviewed, all discussions resolved (waiting on @tjcouch-sil)
There was a problem hiding this comment.
Reviewed 4 of 11 files at r6.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 1 of 8 files at r5.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
This PR satisfies #1213
I implemented sidebar hierarchy and styled the search bar based on Ian's filtering settings Figma. All of one extension's setting groups are on the same page in shadcn
Cards based on the Combined Settings - v0.I added support for receiving a project id and only showing settings for that project.
Other misc changes:
SettingsSidebarContentSearchcomponent, that is similar to theTabNavigationContentSearchcomponent we were using previously, except it uses a ShadcnSidebarfor navigation, and has updated styling. I did not remove the previous component in case that tab navigation is useful somewhere else.SettingsSidebarcomponent with extension list and projectComboBox.ComboBox- I needed to add a style to thePopoverinside of theComboBox, but the className only applied to the trigger, so I added apopoverClassNameandbuttonClassName. And updated the uses.SearchBarCheckBoxtoSwitchin settingsThis change is