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

Congratulations page draft 1 #19

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

brad-lowe
Copy link

Add .svg images and begin congratulations page layout

@netlify
Copy link

netlify bot commented Mar 7, 2022

Deploy Preview for pen-pals ready!

Name Link
🔨 Latest commit 7653a4e
🔍 Latest deploy log https://app.netlify.com/sites/pen-pals/deploys/6286b8d07875b2000893d8e2
😎 Deploy Preview https://deploy-preview-19--pen-pals.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@reginawang3495 reginawang3495 left a comment

Choose a reason for hiding this comment

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

Not exactly sure what you needed help with, but if you wanted to see your Congratulations page, don't forget to make sure that it's rendered on the page, e.g.

 import '../../styles/ExerciseSide.scss';
+import Congratulations from '../Congratulations';

 function ExerciseSide(): JSX.Element {
-  return <section id="exercise-side-container"></section>;
+  return <section id="exercise-side-container">
+    <Congratulations/>
+  </section>;
 }

 export default ExerciseSide;

Anything else I can help with?

@brad-lowe
Copy link
Author

Draft 2, need help with the "Try these next!" text, congratulations text, and the turtle staying where they're supposed to

@8BitRobot
Copy link
Contributor

Hi! So here's what I've found with respect to a working solution for centering the speech bubble:

  1. On #congratulations-container, the units for width and height should be vw for width and vh for height, not vmin. vmin forces both dimensions to be scaled relative to the smaller dimension of the screen, which isn't really what we need here because the primary target audience for this learning lab is in fact people using a computer. We can make it responsive in other ways; this isn't the most effective way to do so. In addition, you can set both width and height to 100vw and 100vh respectively: since this is the container element, it's better for it to fill the entire screen.
  2. Also on #congratulations-container, we can add justify-content: center to make that element centered. This element already has Flexbox enabled, from what I can tell, so there's no need to adjust anything else apart from adding that.
  3. On #speech-bubble-container, which is currently a relatively positioned element, you should add a top attribute (with percentage units) to shift it down the page. This element doesn't need to be centered perfectly because the elements above and below (a.k.a. the header and the dog-cat buttons) aren't positioned uniformly either, so feel free to just eyeball it with respect to how much you shift it down.

Assuming your code looks like mine, the above steps should do well to position the speech bubble and respond to most reasonable screen sizes.

And here's some other stuff I found:

  • You should remove the width attribute from the header image, it's fine to have it at its natural sizing. We're not targeting mobile devices with this learning lab, so (for now) there's no need to make it smaller than it is (and especially not with pixel sizing).
  • Don't use percentage units on the border-radius for the dog and cat boxes; instead, check the Figma for the exact pixel values and just use those. Border radii are typically designed to be exact measurements, not relative or responsive; so it's best to just use pixel values here. Figma will tell you what values you need.

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.

3 participants