-
Notifications
You must be signed in to change notification settings - Fork 146
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: Onboarding flow for dashboard #825
Conversation
@PedroCo3lho is attempting to deploy a commit to the StarknetID Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce a new component, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/[addressOrDomain]/page.tsx (3 hunks)
- components/dashboard/SuggestedQuests.tsx (1 hunks)
Additional comments not posted (2)
app/[addressOrDomain]/page.tsx (2)
378-379
: Conditional rendering ofSuggestedQuests
is appropriate.The use of conditional rendering based on
isOwner
is a good approach to personalize the user experience.
356-369
: Streamlined logic and formatting adjustments are beneficial.The streamlined logic and formatting adjustments enhance code readability and maintainability.
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/dashboard/SuggestedQuests.tsx (1 hunks)
Additional context used
Biome
components/dashboard/SuggestedQuests.tsx
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (3)
components/dashboard/SuggestedQuests.tsx (3)
1-7
: Imports and type declarations are well-structured.The imports are relevant and necessary for the component's functionality.
23-33
: JSX rendering logic is clear and effective.The component structure aligns with the intended UI design and follows best practices.
36-36
: Component export is correctly implemented.The export statement follows common conventions for React components.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Your PR looks really really great (the front is good and responsive, and it works very well, so gg!). I just added a little comment to optimize the quests fetching. Great PR thanks for your work!
useEffect(() => { | ||
const fetchQuests = async () => { | ||
const res = await getQuests(); | ||
if (res && res.onboarding) { | ||
setQuests(res.onboarding.slice(0, 3)); | ||
} | ||
}; | ||
|
||
fetchQuests(); | ||
}, []); |
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.
Just a little optimization here:
As we use quests a lot in the website, it would involve a lot of requests if we had to fetch them everywhere we use them. Instead, we just fetch them once and store them in a shared state.
You can use
const { quests } =
useContext(QuestsContext);
to access it (and them simply keep the 3 firsts).
Tell me if it makes sense to you!
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.
@Marchand-Nicolas Yeah, that makes total sense. Does the new commit 08880e9 solve the problem? I wasn't clear if you wanted me to display the first 3 quests from the entire array or just those from the onboarding category.
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/dashboard/SuggestedQuests.tsx (1 hunks)
Additional comments not posted (2)
components/dashboard/SuggestedQuests.tsx (2)
1-7
: Imports are appropriate and necessary.The imports align with the functionality of the component, ensuring type safety and context usage.
18-29
: Rendering logic is clear and effective.The component effectively renders the title, description, and quest cards using the mapped quests.
const SuggestedQuests: React.FC = () => { | ||
const { quests: contextQuests } = useContext(QuestsContext); | ||
const [quests, setQuests] = useState<QuestDocument[]>([]); | ||
|
||
useEffect(() => { | ||
const onboardingQuests = contextQuests.filter((quest) => quest.category === "onboarding"); | ||
setQuests(onboardingQuests); | ||
}, [contextQuests]); |
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.
Optimize filtering logic.
The filtering logic is clear, but you can optimize it by directly setting the filtered quests in the state initialization.
Apply this diff to optimize the filtering logic:
const SuggestedQuests: React.FC = () => {
const { quests: contextQuests } = useContext(QuestsContext);
- const [quests, setQuests] = useState<QuestDocument[]>([]);
+ const [quests, setQuests] = useState<QuestDocument[]>(contextQuests.filter((quest) => quest.category === "onboarding"));
useEffect(() => {
- const onboardingQuests = contextQuests.filter((quest) => quest.category === "onboarding");
- setQuests(onboardingQuests);
+ setQuests(contextQuests.filter((quest) => quest.category === "onboarding"));
}, [contextQuests]);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const SuggestedQuests: React.FC = () => { | |
const { quests: contextQuests } = useContext(QuestsContext); | |
const [quests, setQuests] = useState<QuestDocument[]>([]); | |
useEffect(() => { | |
const onboardingQuests = contextQuests.filter((quest) => quest.category === "onboarding"); | |
setQuests(onboardingQuests); | |
}, [contextQuests]); | |
const SuggestedQuests: React.FC = () => { | |
const { quests: contextQuests } = useContext(QuestsContext); | |
const [quests, setQuests] = useState<QuestDocument[]>(contextQuests.filter((quest) => quest.category === "onboarding")); | |
useEffect(() => { | |
setQuests(contextQuests.filter((quest) => quest.category === "onboarding")); | |
}, [contextQuests]); |
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.
@Marchand-Nicolas, would it be good to make this changes?
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
Not necessary imo
|
||
return ( | ||
<div> | ||
<div className="text-center"> |
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.
Fixed the rendering order, it now renders directly in the middle.
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.
Lgtm!
Implemented the new onboarding UI provided in Figma : https://www.figma.com/design/fh0OAvj4AS08kHoSxu3DkE/%F0%9F%9A%80-Starknet-Quest?node-id=5408-11294&t=siYIjELkI3CzUjfH-1
Pull Request type
Resolves: #721
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SuggestedQuests
component to guide new users in starting their journey on the Starknet platform.Bug Fixes
Style