-
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
Nada/feat: implementation of copy ads #102
Nada/feat: implementation of copy ads #102
Conversation
Pull Request Test Coverage Report for Build 9401031472Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9403484156Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9406568005Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9406999844Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9407262061Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9407266613Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9407297662Details
💛 - Coveralls |
const labelSize = isMobile ? 'sm' : 'xs'; | ||
const valueSize = isMobile ? 'md' : 'sm'; |
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.
We need to start to consider the tablet view.
const labelSize = isMobile ? 'sm' : 'xs'; | |
const valueSize = isMobile ? 'md' : 'sm'; | |
const labelSize = isDesktop ? 'xs' : 'sm'; | |
const valueSize = isDesktop ? 'sm' : 'md'; |
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.
since this uses the same components as the ones in create/edit ad it might break the existing ones.
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.
probs we need to create another card to check the responsive sizings for the whole of v2
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.
yeah.. also we might need design.. because for the copy ads i tried the mobile design for tablet and it looks really bad..
Pull Request Test Coverage Report for Build 9412383731Details
💛 - Coveralls |
const CopyAdForm = ({ | ||
formValues, | ||
isModalOpen, | ||
onClickCancel, |
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't we use the onRequestClose
for this? 🤔
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 have removed the prop. its not needed.
Pull Request Test Coverage Report for Build 9444143006Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Preview Link: https://nada-FEQ-2197-copy-a.deriv-p2p-app.pages.dev
|
Pull Request Test Coverage Report for Build 9445264013Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Screen.Recording.2024-06-06.at.3.51.43.PM.mov
Screen.Recording.2024-06-06.at.11.13.49.PM.mov
Copy advert feature
Fix issue with validation in create/edit ad
Fix order completion time not passed in create/edit ad