[General] UX improvements in loading screens #1112
Conversation
| @@ -28,7 +34,7 @@ export default function ToggleSwitch(props: Props) { | |||
| aria-label={title} | |||
| /> | |||
|
|
|||
| <span {...props} className="checkmark" /> | |||
| <span {...checkmarkProps} className="checkmark" /> | |||
There was a problem hiding this comment.
What is this change doing exactly?
I believe that if we pass a prop that is not listed, it will be ignored.
so we need to check the props we use when calling this component :)
There was a problem hiding this comment.
const {
handleChange,
value,
disabled,
title,
dataTestId = 'toggleSwitch',
...rest
} = propsare by any chance thinking of something like rest property? Because destructuring an object doesn't remove destrustured properties.
anyway, I removed properties which caused errors: handleChange and disabled, but I kept the rest just in case the attributes are used somewhere and I didn't want to break anything.
| }: UpdateComponentProps) { | ||
| const componentRef = useRef<HTMLDivElement>(null) | ||
| const wrapperRef = useRef<HTMLDivElement>(null) | ||
| const observer = useMemo(() => { |
There was a problem hiding this comment.
Interesting, so like this we can check the size of the component and render always in the center?
There was a problem hiding this comment.
.UpdateComponent element has always height of 100%. Then we change how much of the .UpdateComponent is visible on the screen (ratio) and then change .UpdateComponent__wrapper's height to the ratio.
src/screens/Game/GamePage/index.tsx
Outdated
| @@ -422,12 +422,12 @@ export default function GamePage(): JSX.Element | null { | |||
| </div> | |||
| </> | |||
| ) : ( | |||
| <UpdateComponent /> | |||
| <UpdateComponent message={t('loading.default')} /> | |||
There was a problem hiding this comment.
Perhaps we could use the default message directly into the component in case message is undefined. And make the message a optional prop (I believe might be already)
There was a problem hiding this comment.
I've tried that, but it's not possible (or at least I don't see a simple way to do this), because the UpdateComponent is rendered as the translations are loading.
There was a problem hiding this comment.
My bad, it's actually is possible. Commited the change
flavioislima
added
the
pr:ready-for-review
| ...rest | ||
| }: UpdateComponentProps) { | ||
| const { t } = useTranslation() | ||
| if (message === undefined) { |
There was a problem hiding this comment.
I believe a better check would be:
if (!message) {
because then it would use the default also if we send an empty string, null or any other falsy value.
There was a problem hiding this comment.
That's deliberate:
a) It's inline with current behavior because default value only work if the property is missing (undefined). You can pass empty string to the component and it won't display default message.
b) I can't see any reason to restrict ability to pass empty string and display icon without any message
c) You can't pass any falsy value, because type checking won't allow it. You can either pass no value, which will cause displaying default value or empty string which will display no message
There was a problem hiding this comment.
Ah, I see. Makes sense to load only the icon.
Good then.
Various UX improvements in loading screens:
Use the following Checklist if you have changed something on the Backend or Frontend: