-
Notifications
You must be signed in to change notification settings - Fork 159
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
[WIP] Dialog setting #2032
[WIP] Dialog setting #2032
Conversation
(not used currently in codebase)
</> | ||
); | ||
} | ||
interface AvailableLanguages { |
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.
duplicate from
export const AvailableLanguages = { |
|
||
const SelectItem = React.forwardRef(({ children, ...props }: any, forwardedRef) => { | ||
return ( | ||
<Select.Item {...props} id="itemInner" ref={forwardedRef} |
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.
You don't need to forwardRef a sub-component at the top of the tree
@@ -36,10 +43,48 @@ class Settings extends React.Component<IProps, undefined> { | |||
<LibraryLayout | |||
title={__("header.settings")} | |||
> |
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 component is loaded from react-router
thorium-reader/src/renderer/library/routing.ts
Lines 62 to 66 in 1f0deca
"/settings": { | |
path: "/settings", | |
// exact: false, | |
component: Settings, | |
} as Route, |
I think that the react-router route /setting
should be disable (the setting component is no longer a page but a modal dialog now). And Setting Dialog must be move to the common/components/dialog
folder (used in library and reader as well)
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.
I think that the react-router route
/setting
should be disable (the setting component is no longer a page but a modal dialog now).
Nested routes (i.e. relative settings_modal
path inside another route definition) would work well declaratively, for example /bookshelf
+ nested /bookshelf/settings_modal
... but the settings modal popup dialog with Radix is really designed to be displayed anywhere (overlaid on top of an existing route component) where a "call site" button (trigger UI control) can be placed, so I agree that using the router to implement this is a waste of time and effort (unless we really want to use router functionality, for example to open the settings dialog in a specific tab by passing route parameters?)
src/renderer/library/components/settings/ReadingOptions/TextContent.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre Leroux <[email protected]> Arthur Le Meur <[email protected]>
Co-authored-by: arthur-lemeur <[email protected]>
…/thorium-reader into arthur/ui/settings-dialog
…eader,lib) Will be used by the next reader setting modal PR #2032
replaced by #2039 |
No description provided.