refactor(luna): impl loading and error in lessons page #254
Conversation
moalidv
force-pushed
the
mo/luna-loading-error
branch
5 times, most recently
from
2532f6a to
5ce14d6
Compare
moalidv
force-pushed
the
mo/luna-loading-error
branch
from
5ce14d6 to
d885874
Compare
There was a problem hiding this comment.
I think it would be better when you aren't using the svg, to not include it
| @@ -79,7 +79,9 @@ describe("Ratings", () => { | |||
| values: [1, 2, 4, 5], | |||
| }); | |||
|
|
|||
| const avg = await ratings.findAvgRatings([t1.id, t2.id, t3.id]); | |||
| const avg = await ratings.findAvgRatings({ | |||
There was a problem hiding this comment.
unrelated changes, Try to rebase on latest master
There was a problem hiding this comment.
@mostafakamar2308
I want to ask about that point
findAvgRatings function take an object as a parameter containing users as an array of numbers so how can we give the function this array directly without the object and label of users
There was a problem hiding this comment.
@mostafakamar2308
I have tried to rebase on the latest master but the error is still present
There was a problem hiding this comment.
If you rebased now, you should not face this issue.
|
@moalidv please rebase. |
| ${variant === "large" ? "30px" : ""} | ||
| ${variant === "medium" ? "25px" : ""} | ||
| ${variant === "small" ? "15px" : ""}, white | ||
| ${variant === "large" ? "16px" : ""} | ||
| ${variant === "medium" ? "0px" : ""} | ||
| ${variant === "small" ? "0px" : ""} |
There was a problem hiding this comment.
we can have a function that returns the size based on the variant.
const size = useMemo(() => {
if (variant === 'large') return '30px';
// ...
}, [])
// or even better
const styles = useMemo(() => {
if (variant === 'large') return ['30px', '16px'];
// ...
}, [])There was a problem hiding this comment.
This is not in use. Please remove it as well as its export in the package.json file.
|
There is a degree of distortion in the spinner. Screencast.from.2024-12-30.20-06-47.webm |
| @@ -25,8 +25,9 @@ export const Content: React.FC<{ | |||
| fetching: boolean; | |||
| error: boolean; | |||
| hasMore: boolean; | |||
| refetch?: Void; | |||
There was a problem hiding this comment.
I think using the Void type, in general, is considered bad or harmful practice. If it's not feasible or not that necessary and tedious to declare/define a type for this specific function. I'd recommend leaving a comment stating what it's supposed to do.
There was a problem hiding this comment.
You may write, for instance: // A callback function that invokes tanstack query refetch method.
| export const LoadingError: React.FC<{ | ||
| retry: Void; | ||
| error: string; | ||
| variant?: "small" | "medium" | "large"; |
There was a problem hiding this comment.
Awesome work. This solution is really good and flawless. However, I have a suggestion: what if we defined the type of variant as follows "10" | "16" | "20", and then embedded it in tailwind class name tw-w-${variant}. I think It could make the code more succinct.
There was a problem hiding this comment.
This will not work due to how tailwind scan for classnames.
There was a problem hiding this comment.
This will not work due to how tailwind scan for classnames.
I think It will work fine, at least inside the cn utility function.
|
This pull request will be closed in favor of #271 |
No description provided.