Closed alexandercyu closed 5 months ago
The spacing format is 50px-[Heading-30px-Body-30px-Button]-50px-[Heading-30px-Body-30px-Button]. For technology card -50px-Heading-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button] Notice the 24px margin on right and left side on the screen. Margin on right side of the screen is much higher than 24px All heading should be in the same line "About Marble" and "Getting Started" doesn't seem to be in one line. Student , Researcher and Hobbyist section has a different format.
Please compare all components of the Figma design to align with the design.
What about the spacing of h6 headers? They are mostly consistent with 30px below in the desktop design and 20px below in the mobile design. But the quickstart list has 30px in desktop and mobile and the "Explore the Services" title doesn't have any space below. I think for consistency the quickstart list should use a different font class. I don't think an h6 should have been used there to begin with since it's not a heading.
Updated buttons so regular sized buttons are visible on desktop and medium sized buttons are visible on mobile as per PR #125
The site no longer builds (jinja throws an error). Please fix and then request a review again.
Fixed the error with jinja not building.
This has changed:
These sections are too close together:
The feedback button aligns weirdly at some screen widths:
Fixed the banner issues and added some margins for the spacing issues.
These sections are too close together:
This still isn't fixed
And the feedback button should not move beside the text until the feedback section stacks on top of the useful links section
Added spacing between the heading and the sections.
Adjusted when the feedback button shifts position.
It's still there
Is it not clear what the issue is? The button is too close to the header on the next line.
This happens when the screen size is
md
I think
I didn't see it because it only happens on one screen size. Fixed it now.
The gap between heading title body and button is all the same in technology card! I have mentioned in my previous review about the spacing format for technology card . Please check my previous review and the screenshot attached with this review and make the changes accordingly.
This note at the top when the PR was created explains it.
NOTE: 30px spacing below h3 and h6 titles have been made in another branch. Didn't want to duplicate the change here
But I guess I will duplicate it in this branch after all.
But I guess I will duplicate it in this branch after all.
Please don't. Let's merge that other branch first and then we can look at how this affects this branch.
But I guess I will duplicate it in this branch after all.
Please don't. Let's merge that other branch first and then we can look at how this affects this branch.
Wish I'd seen this first. I already did it.
NOTE: there is a 2.5em height applied previously when developing the desktop site to the h6 title so the content underneath it is pushed down consistently across cards.
The spacing format is 50px-[Heading-30px-Body-30px-Button]-50px-[Heading-30px-Body-30px-Button]. For technology card -50px-Heading-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button] Notice the 24px margin on right and left side on the screen. Margin on right side of the screen is much higher than 24px All heading should be in the same line "About Marble" and "Getting Started" doesn't seem to be in one line. Student , Researcher and Hobbyist section has a different format. Please compare all components of the Figma design to align with the design.
What about the spacing of h6 headers? They are mostly consistent with 30px below in the desktop design and 20px below in the mobile design. But the quickstart list has 30px in desktop and mobile and the "Explore the Services" title doesn't have any space below. I think for consistency the quickstart list should use a different font class. I don't think an h6 should have been used there to begin with since it's not a heading.
The headers H6 in mobile design has 30px gap bellow in desktop and mobile both. The Quickstart menu design remains same at the browser design. And yes, we should change Quickstart menu items to Subtitle 2 instead on H6 on browser and mobile. I have made these changes in the design and added a comment.
About Marble - Heading missing
Don't see the change in this section, needs to match the design.
Menu Text drop occurring on Tablet size
please check if this gap is applied.
This is likely because the conflicts have not been resolved yet.
About Marble - Heading missing Don't see the change in this section, needs to match the design. Menu Text drop occurring on Tablet size please check if this gap is applied.
This is likely because the conflicts have not been resolved yet.
Most of the issues above were due to merge conflicts, which have been resolved.
please check if this gap is applied.
I've added this spacing.
Maybe I missed it, but this change was made to the design last week and no notification/comment was made regarding it.
The spacing format is 50px-[Heading-30px-Body-30px-Button]-50px-[Heading-30px-Body-30px-Button]. For technology card -50px-Heading-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button]-30px-[Subheading-20px-Body-20px-Button] Notice the 24px margin on right and left side on the screen. Margin on right side of the screen is much higher than 24px All heading should be in the same line "About Marble" and "Getting Started" doesn't seem to be in one line. Student , Researcher and Hobbyist section has a different format. Please compare all components of the Figma design to align with the design.
What about the spacing of h6 headers? They are mostly consistent with 30px below in the desktop design and 20px below in the mobile design. But the quickstart list has 30px in desktop and mobile and the "Explore the Services" title doesn't have any space below. I think for consistency the quickstart list should use a different font class. I don't think an h6 should have been used there to begin with since it's not a heading.
The headers H6 in mobile design has 30px gap bellow in desktop and mobile both. The Quickstart menu design remains same at the browser design. And yes, we should change Quickstart menu items to Subtitle 2 instead on H6 on browser and mobile. I have made these changes in the design and added a comment.
Changed quickstart menu items to subtitle 2 font class.
About Marble - Heading missing Don't see the change in this section, needs to match the design. Menu Text drop occurring on Tablet size please check if this gap is applied.
This is likely because the conflicts have not been resolved yet.
Most of the issues above were due to merge conflicts, which have been resolved.
please check if this gap is applied.
I've added this spacing.
Maybe I missed it, but this change was made to the design last week and no notification/comment was made regarding it.
Please note: Any changes made last week have been left a comment along.
Not sure what are you referring to Usage menu as? Marble title is white in colour on mobile version, please apply standard colour scheme to the title.
This is done in the marble-mobile-node branch
@ShrutiK100 I'm going to assume you've been following the messages here and are fine with the changes so far. I'm going to merge this in and any further review will be in the final review branch.
NOTE: FAQ section changes have been made in the marble-mobile-about branch NOTE: 30px spacing below h3 and h6 titles have been made in another branch. Didn't want to duplicate the change here.