-
Notifications
You must be signed in to change notification settings - Fork 2
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
JTE/PKFE-20 #46
JTE/PKFE-20 #46
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.
@@ -26,7 +27,15 @@ interface Props { | |||
*/ | |||
export const BaseLayout: React.FC<Props> = ({ children }) => { | |||
const Theme = useTheme(); | |||
const ThemeContext = useThemeContext(); | |||
|
|||
const [isOptionsMenuOpen, setIsOptionsMenuOpen] = useState(false); |
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.
Is it menu or a dialog? In components it's defined as dialog. If it's a dialog why its in modal folder? :p Strange naming convention
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.
Some sources say a dialog is a modal, others say modal is a dialog.
icon={<LightMode sx={{ width: '1.5rem', height: '1.5rem', color: Colors.backgroundPrimaryLight }} />} | ||
onClick={() => ThemeContext.update()} | ||
/> | ||
)} | ||
</Box> | ||
</Box> | ||
<Box sx={{ width: '95.75%', height: '99.5%', borderRadius: '0.625rem', bgcolor: Theme.palette.secondary.main }}> |
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.
Strange width
and height
. Might be a problem in the future.
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 don't see how it could be problematic.
app/front-end/src/components/modals/settingsDialog/settingsFields/timeZoneSetting.tsx
Show resolved
Hide resolved
app/front-end/src/components/modals/settingsDialog/settingsSelectField.tsx
Show resolved
Hide resolved
app/front-end/src/components/modals/settingsDialog/settingsSelectField.tsx
Show resolved
Hide resolved
app/front-end/src/components/modals/settingsDialog/settingsSelectField.tsx
Show resolved
Hide resolved
fontSize: '0.9rem', | ||
}, | ||
}, | ||
}, |
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.
Currently not sure about this. If we would have defined components and their styles, yes I would agree. we can keep it but keep in mind to limit this usage. Only basics like typography
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 it would be good to have consistant select fields and menus across all of the project
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.
Good work!
Implemented settings menu
(colors may slightly change after the color update PKFE-25 pull request merge)