Skip to content
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

Jingwei/982 onboarding card compoent #1011

Merged
merged 9 commits into from
Apr 23, 2023

Conversation

sky1122
Copy link
Contributor

@sky1122 sky1122 commented Apr 12, 2023

changes:
1. create OnboardingCard file in website/src/modules
2. create index.js
3. create OnboardingCard.js
4. StyledOnboardingCard.js
Issue:
1. import { Typography } from '@devlaunchers/components/components/atoms' seems I didn't use It correctly, so the font is not working.

How to use the component:
the demo of how to use this component is in user-profile/src/UserOnboardingModal/UserOnboardingModal.js

image

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

⚠️ No Changeset found

Latest commit: fa2272e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sky1122 sky1122 requested review from judeamos and HavreLoic and removed request for Enjoy2Live and pyxld-kris April 12, 2023 04:03
Copy link
Contributor

@judeamos judeamos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed your changes @sky1122, and left feedback below.

@@ -38,6 +41,22 @@ export default function UserOnboardingModal({isOpen}) {
<ModalBody>
<Typography type="p"> body</Typography>
{/* Onboarding Card Component */}
{/* passing a prop to decide show checked or unchecked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these comments to the OnboardingCard.js file.

iconImg,
title,
subtitle,
checked,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this prop to "completed", this name follows the user story better, and reflect where necessary.

subtitle,
checked,
}) {
console.log({ iconImg });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove console logs




<CheckedWrapper >
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway of removing this "CheckedWrapper"?

  • I see you have added a padding-right: 34px, you can move this style into "OnboardingCardLayer".
  • The align-self: centre can be moved into the "OnbordingCardLayer" as align-items: centre.

display: flex;
justify-content: space-between;
radius: 10px;
width: 686px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set width to 100%, should be responsive.

height: auto;
position: relative;
border-radius: 10px;
border: 1px solid #000000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

border is not needed.

export const OnboardingCardLayer = styled.div`
display: flex;
justify-content: space-between;
radius: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have radius & border-radius used here, should it not be radius?

console.log({ title });
console.log({ checked });
return (
<OnboardingCardLayer checked = {checked}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"OnboardingCardContainer" Would be a better name/description.

<OnboardingCardLayer checked = {checked}>
{/* <img src={iconImg} alt="icon" /> */}
<PicWrapper>
<TextWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "TextWrapper" is not needed here. You can add your paddings to the "PicWrapper" styled component. You can also utilise:

- gap: 10px; gap: 10px 20px; /* row-gap column gap */ row-gap: 10px; column-gap: 20px; css properties for flex boxes.

)

export const IconImg = ({iconImg}) => {
console.log("style");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove console.logs

@sky1122 sky1122 requested a review from judeamos April 20, 2023 16:09
@sky1122
Copy link
Contributor Author

sky1122 commented Apr 20, 2023

I make these changes, however, because the tag in the library cannot change the color, and after the typography component get this works. The color will be able to change.

Copy link
Contributor

@HavreLoic HavreLoic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jingwei there is a merge conflict in apps\user-profile\src\components\modules\UserOnboardingModal\UserOnboardingModal.js I think that once it is solved this branch is good to go

@sky1122
Copy link
Contributor Author

sky1122 commented Apr 23, 2023

I resolve this conflict, please take a look when you are free.

@sky1122 sky1122 requested a review from HavreLoic April 23, 2023 14:50
Copy link
Contributor

@judeamos judeamos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two changes i need you to make. I've approved the PR, once you pushed the changes, you can then merge yourself 👍🏾

@@ -18,7 +18,8 @@ export default function UserProfilePage() {
* More conditions will be applied when modal should be opened in the future.
*/
const openUserOnboardingModal = () => {
return !(userData && userData.username)
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert your changes here. Do not push me.js

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just uncomment return !(userData...) , and delete the return true

import React from 'react'
import { Typography } from '@devlaunchers/components/components/atoms'
import { OnboardingCardContainer, PicWrapper, TextWrapper, CheckedWrapper, CheckedSVG, IconImg } from './StyledOnboardingCard'
export default function OnboardingCard({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add a component comment describing what the component does:
Screenshot 2023-04-23 at 19 37 40

@sky1122 sky1122 merged commit a1acc2c into development/user-profile Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants