-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(welcome_modal): added welcome modal #18
feat(welcome_modal): added welcome modal #18
Conversation
This adds a welcome modal
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.
Hey @Shaiq1217,
Thank you for the PR, amazing work 🦾🎉
I left a couple of comments, could you check when you have time, please?
top: 50%; | ||
left: 50%; | ||
transform: translate(-50%, -50%); | ||
width: 700px; |
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.
align-items: flex-start; | ||
`; | ||
|
||
export const WelcomeModal = styled(Box)( |
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.
import { Box, css } from '@mui/material'; | ||
import InfoClosed from '../CloseIcon/InfoIcon'; | ||
|
||
export const CloseIcon = styled(InfoClosed)``; |
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.
❔ question:
Could you please elaborate on why we need empty styled wrapper?
What is the benefit of it?
It looks redundant but maybe I'm missing something.
import { Modal } from '@mui/material'; | ||
|
||
const WelcomeModal = () => { | ||
const [isOpen, setIsOpen] = useState(true); |
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.
💭 thought:
I agree that the information modal can be helpful to add some context for new users. However, for existing users, a popup after every page refresh seems more destructive than helpful from the UX point of view.
What do you think about the idea of showing it only once and storing the flag in localStorage?
are executed under the hood. | ||
</p> | ||
<p> | ||
You can also add callbacks to the Web API, requestAnimationFrame, and |
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.
🤓 nitpick:
Probably, avoiding multiple uses of "you can add" in a row could make the text a little bit better.
For example:
Add code snippets to the editor and run them to see their execution under the hood.
You can also experiment with Web API callbacks and requestAnimationFrame to visualize how they are processed in the event loop.
…nfo modal Added responsive media queries to shared info modal fix# 17
pulled from master to incorporate recent changes
I'm closing the PR since there have been no updates for more than 1 month. |
This pull request introduces a new
WelcomeModal
component to the application. The changes include importing and integrating the modal in the main app, as well as defining its styles and behavior.Definition of Welcome Modal Styles:
src/components/WelcomeModal/WelcomeModal.styled.ts
: Added styled components forWelcomeModal
, includingCloseIcon
,ModalHeader
, and the modal itself using@emotion/styled
and@mui/material
.Implementation of Welcome Modal Component:
src/components/WelcomeModal/WelcomeModal.tsx
: Created theWelcomeModal
component with state management for its visibility, and included a welcome message and instructions for using the event loop explorer.