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

Fix: Style for onboarding flow #2516

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Jun 11, 2024

Description

Refactor landing page item to render children
Fix styling issue on smaller devices

Testing

TODO: Button styling issue for actionable item list on landing page.

  • Step 1. Open landing page
  • Step 2. Check the actionable item list on both mobile and desktop

Resolves #2111

@mmioana mmioana self-assigned this Jun 11, 2024
@mmioana mmioana requested review from a team as code owners June 11, 2024 10:48
@CLAassistant
Copy link

CLAassistant commented Jun 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@melyndav melyndav left a comment

Choose a reason for hiding this comment

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

Hey @mmioana congrats on getting started with your first PR.

The scope of the issue is to make the landing page, profile creation and colony creation flows responsive for mobile.

Currently, only desktop was built for these and they are not optimised for our mobile users. Tablet also needs to be considered as part of this scope.

In the issue you'll find a Figma link that shows the complete mobile flows and the landing page variations. Please let me know if you'd like a call to walk through what's needed for this issue. 👍

@mmioana mmioana marked this pull request as draft June 11, 2024 13:34
@mmioana mmioana force-pushed the fix/2111-mobile-onboarding branch from 9bc521d to 83ee976 Compare June 13, 2024 12:29
@mmioana mmioana changed the title Fix: Style for landing page items Fix: Style for onboarding flow Jun 13, 2024
@mmioana mmioana marked this pull request as ready for review June 13, 2024 12:51
Copy link

@melyndav melyndav left a comment

Choose a reason for hiding this comment

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

Hey again @mmioana nice start on this one! A big one to tackle.

Some feedback below to align the UI with the Figma designs and existing mobile interactions:

Landing page

  1. The padding between the 'Colony app' title and the 'Welcome to Colony' should match Figma.
image
  1. The blue pill should be removed on mobile as per Figma
image
  1. The icon and background box should be aligned to the top of the content block to match the Figma design.
image

Flow

  1. The stepped navigation pills are missing the background styling.
image
  1. The buttons should be styled to match Figma.
image image
  1. I have my wallet connected, but the header doesn't display the connected state. I should see the chain, my avatar and the settings icon as per the Figma designs.
image
  1. Padding between these two select boxes should match Figma.
image
  1. These input fields should be aligned according to Figma (stacked for better touch interactions)
image
  1. The button buttons should be sticky and the content scrolls.

  2. These radios should use the existing component and match the desktop styling

image image
  1. Can you make this button in the bottom widget within the modal full width?
image

rdig
rdig previously approved these changes Jun 13, 2024
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Codewise this looks good. I like the desktop / mobile sidebar refactor into a separate component, and the introduction of enums for the flow types.

However, please take care of the styling issues that were pointed out by Mel, before merging this

Screenshot from 2024-06-13 18-53-22

const LandingPage = () => {
const [, setHoveredItem] = useState<number>(1);
const [, /* hoveredItemIndex */ setHoveredItemIndex] = useState<number>(1);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a leftover from debugging ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As only one item on the page is enabled and setter is already used, thought to comment the actual state to use it after we enable the other items

headingText={MSG.createColonyTitle}
headingDescription={MSG.createColonyDescription}
icon={Layout}
// imgSrc={CreateAColonyBanner}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

}
headingDescription={MSG.viewUserProfileDescription}
icon={UserCircle}
// imgSrc={CreateAProfileBanner}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Refactor landing page item to render children
Fix styling issue on smaller devices

Resolves #2111
Use stepper component for mobile wizard sidebar
Adjust stepper component to snap to active step item
Refactor onboarding helper to compare against enum values
Fix styling issue on smaller devices
const Icon = icon || ICON_NAME_MAP[stage];
const isMobile = useMobile();

useEffect(() => {
if (ref?.current && isHighlighted) {
ref.current.scrollIntoView({ behavior: 'smooth', inline: 'end' });
Copy link
Contributor

@rumzledz rumzledz Jun 14, 2024

Choose a reason for hiding this comment

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

Great start on your 1st PR @mmioana ! 🥳

Code-wise all good apart from the ones mentioned! This is not a change request but thought I'd say this would be pretty cool as a hook i.e.

const { ref } = useScrollIntoView({ scrollCondition: yourCondition, ...otherParams })

or maybe expose a scroll function for a more imperative approach

const { ref, scroll } = useScrollIntoView();

if (condition) {
  scroll()
}

or a mixture of both! 😄

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Smashing work on your first PR, immediately a refactor, nice 😎
Codewise it looks good, will approve after the UI issues have been resolved!

Fix styling changes on smaller devices after review
Use custom useMediaQuery for mobile and tablet
Update button row styling
Refactor declarative scroll into view behaviour into custom hook
Fix styling for checked radio buttons
@mmioana mmioana requested a review from melyndav June 17, 2024 10:12
Copy link

@melyndav melyndav left a comment

Choose a reason for hiding this comment

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

Hey @mmioana,

Nice work on the requested updates! Just one minor change is needed to align it with the Figma designs.

  1. This heading doesn't need to be shown and isn't in the designs, so please remove and match the paddings accordingly:
image

Thanks again for your continued work on this! 🐞

Remove sidebar title on mobile
Remove bottom margin
@mmioana mmioana requested a review from melyndav June 17, 2024 13:00
Copy link

@melyndav melyndav left a comment

Choose a reason for hiding this comment

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

Thanks for the final fix on this PR @mmioana, appreciate your work! 🎉

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice changes, good job handling the myriad of required CSS fixes! 💪

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.

Mobile & Tablet: Colony creation and profile onboarding flows
6 participants