-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add help screen layout #116
Conversation
c19113b
to
8dafda4
Compare
0a72fb4
to
32aa454
Compare
4631c02
to
aa517fe
Compare
that lib throws some warnings |
Good point. Will migrate to SwiperJs. As |
disabled: boolean | ||
} | ||
|
||
const ParaTimeSelectorGlow = styled(Box, { | ||
shouldForwardProp: (prop: string) => | ||
!(['disabled'] as (keyof ParaTimeSelectorProps)[]).includes(prop as keyof ParaTimeSelectorProps), | ||
})<ParaTimeSelectorProps>(({ disabled, theme }) => ({ | ||
!(['disabled'] as (keyof ParaTimeSelectorBaseProps)[]).includes(prop as keyof ParaTimeSelectorBaseProps), |
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.
do we even need shouldForwardProp
for disabled attr?
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 rather leave it, for cases where [disabled]
style selector is globally applied, to not conflict with styling.
return false | ||
} | ||
|
||
switch (step) { |
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.
why are we using switch statement to handle boolean ? Can't we just write
step === ParaTimeSelectorStep.ShowHelpScreen
. Additionally, do we even need utils for such simple case?
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 thought that mobile version would be more complex. Simplified the simple switch
statements to if
s, but I would keep the utils. If you feel strongly against I can move it into component.
// declaration in d.ts file declaration not correctly recognized | ||
declare global { | ||
namespace JSX { | ||
interface IntrinsicElements { |
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.
can we move it to src/types file?
|
||
useEffect(() => { | ||
const handleSlideChange = (e: Event) => { | ||
const [swiper] = (e as SlideChangeEvent).detail |
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.
Are these types correct? handleSlideChange e param is a type of SlideChangeEvent not Event, right?
src/index.tsx
Outdated
import { register } from 'swiper/element/swiper-element' | ||
// register Swiper custom elements | ||
register() |
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.
How come we aren't using 'swiper/react'?
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.
ah, i see, "Swiper React components will likely to be removed in future versions. It is recommended to migrate to Swiper Element instead."
d95d4b4
to
2d8f99a
Compare
component="img" | ||
sx={{ | ||
display: 'block', | ||
width: '100%', |
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 is causing issue @lukaw3d was showing
Solution
Add help screen on mobile version
SwiperJs
dependency for carousel effect;Links