-
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
Adding simple customer promt screen #382
Conversation
…dule-onboarding into add/sitedetails/simple
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.
scrnli_13_12_2023_15-57-09.mp4
- There is a BUG that needs to be addressed and THE DATA IS NOT SAVED.
- Please add Bottom Margin in Mobile View.
- NEXT Needs to be disabled as it was done on the Site Details page.
- I think the Text should be WHITE and not gray after the user types it.
- Let's remove unnecessary spaces and fix typos to make it better formatted for the user.
Thanks.
src/OnboardingSPA/components/TextInput/TextInputSiteGen/simple/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/steps/SiteGen/SiteDetails/walkthrough/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/components/Button/ButtonWhite/SiteGen/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/components/Button/ButtonWhite/SiteGen/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/steps/SiteGen/SiteDetails/walkthrough/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/steps/SiteGen/SiteDetails/walkthrough/index.js
Outdated
Show resolved
Hide resolved
src/OnboardingSPA/steps/SiteGen/SiteDetails/walkthrough/index.js
Outdated
Show resolved
Hide resolved
PR changes
lint fixes
…dule-onboarding into add/sitedetails/simple
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.
Bug: The placeholder text appears in white, giving the impression that the input is filled. Can we match it to Figma's specifications?
https://github.com/newfold-labs/wp-module-onboarding/assets/38878906/83036744-0055-4370-a031-99a975f84113
Bug: The selected button grays out, giving a false indication that another option was selected. Can we align this with Figma's specifications?
Bug: Can we add some padding between the text and the textarea border?
Bug: I cannot proceed to the next screen when in this state. Additionally, the highlighted white color suggests 'Yes' was selected, but I have actually chosen 'No,' which is now grayed out.
Could we add an appropriate placeholder for this input?
https://github.com/newfold-labs/wp-module-onboarding/assets/38878906/01732971-2644-4187-8545-055ab6f27e4c
When we go back, should we return to the simple prompt screen or the welcome screen?
Bug: In the mobile view, the textarea blurs into the footer. Can we add some spacing or adjust the layout to address this issue?
I will review more once these issues are addressed. Thanks, @girish-lokapure.
src/OnboardingSPA/components/Button/ButtonWhite/SiteGen/index.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,45 @@ | |||
import { useRef, memo } from '@wordpress/element'; | |||
|
|||
const TextInputSiteGenSimple = ( { |
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.
Is it good practice to split this into two components, TextInputSiteGen and TextAreaSiteGen? The component naming structure we follow is components//index.js. Can we eliminate the use of terms such as 'simple,' 'complex,' etc.? Any component directory name should be pascal cased
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're skipping this placeholder for now. We will add it once we get it.
Flow data API keeps failing when I navigate to the customer details step.
We need to sanitise and trim the input before we append things to the prompt
Can we please check color schema for the light and dark mode ensuring good contrast. We also need to move any hardcoded colors to _branding.scss under the respective class name.
I filled only one input in the simple customer details screen and this is the prompt that was generated. Here, if you notice I am able to navigate forward, but when I go again to the simple customer details screen, then forward navigation is disabled. Should we keep the navigation behaviour consistent in both the screens.
The disabled input gets enabled when I switch to mobile view. Can we please check this as well?
…bs/wp-module-onboarding into add/sitedetails/simple
…bs/wp-module-onboarding into add/sitedetails/simple
…odule-onboarding into add/sitedetails/simple
|
…dule-onboarding into add/sitedetails/simple
…dule-onboarding into add/sitedetails/simple
dependency: newfold-labs/wp-module-onboarding-data#20