-
Notifications
You must be signed in to change notification settings - Fork 14
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
UI: Onboarding UI updates #2931
Conversation
067aef9
to
047206b
Compare
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.
Really cool work on this one @rumzledz 🥇
I have tested both the Create colony with missing profile
and Create colony with leela profile
flows
Opening the colony selector
Only a couple of design remarks and one naming comment 🫠
topContent?: ReactNode; | ||
header?: ReactNode; | ||
mobileAndDesktopLayout?: boolean; |
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 we rename this property into something like isSmPaddingEnabled
or applySmPadding
or isSmStylingEnabled
if we want to be a bit more general? I feel mobileAndDesktopLayout
doesn't convey enough information as in why to use it
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.
Thanks for reviewing @mmioana !
When you complete the create profile/create colony flow, the bubble next to the Complete step is invisible
I shall investigate and fix this, great spot! 👌
Can we rename this property into something like isSmPaddingEnabled or applySmPadding or isSmStylingEnabled if we want to be a bit more general? I feel mobileAndDesktopLayout doesn't convey enough information as in why to use it
Tbh none of those variable names also convey what I'm trying to achieve here hahaha 😂 variable names are so difficult! It's not really a matter of applying padding on sm
. It's a matter of varying the breakpoints primarily between mobile and desktop because by default, we are varying between md
and everything above md
. Happy to hear another batch of variable name suggestions and from everyone as well!
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.
There is no header padding if you go to http://localhost:9091/go/{COLONY_NAME_HERE}
As for this, I'll see if this is relevant to my PR. If it isn't I will open a separate issue 👌
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.
@mmioana I've given it a longer name now 😂 I dunno if you're okay with it but would like to get your thoughts tomorrow! Oh and I've fixed the bugs you raised, thanks for that 👌
3e89c44
to
b42ec50
Compare
047206b
to
289ed32
Compare
topContent?: ReactNode; | ||
header?: ReactNode; | ||
// When set to true, the PageLayout will apply sm vs desktop styles. As opposed to md vs desktop styles. | ||
restrictLayoutToMobileAndDesktopBreakpoints?: boolean; |
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.
Way better than before @rumzledz 🥇 though I believe it's more an enablement than a restriction 🙈
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.
hahaha all right all right let's meet halfway, I shall call it enable! 🙌
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.
@mmioana pushed! 🚀 I hope the variable name is much better now 🤞
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.
Thank you for the changes @rumzledz 🙏
Can confirm the complete step bubble is now visible ✅
However regarding the title padding, I noticed if you go below 767px as the screen width, the title is right below the header, but feel free to open another issue for this one
bdd36c4
to
fc8b2c6
Compare
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.
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.
fc8b2c6
to
0e4d5fb
Compare
0e4d5fb
to
ab91e51
Compare
Description
We now have a new sidebar for our Onboarding pages. We are now only handling 2 breakpoints for our Onboarding pages:
sm
and non-sm
.Testing
Note
The Feedback Button will be developed separately
The Colony Switcher UI will be completed separately
Colony Creation Flow
node scripts/create-colony-url.js
Account Creation Flow
Diffs
Changes 🏗
WizardSidebar
is now just calledWizard
MobileWizardSidebar
is now calledMobileWizardHeader
PageLayout
has been historically designed to switch between tablet and desktop. But it now has aenableMobileAndDesktopLayoutBreakpoints
boolean prop which makes it switch between mobile and desktop views insteadResolves #2889 & #2887