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

Exercise side #20

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

Exercise side #20

wants to merge 8 commits into from

Conversation

abishan1
Copy link

@abishan1 abishan1 commented Mar 7, 2022

Summary

#4 added the level text to the Exercise Side

@netlify
Copy link

netlify bot commented Mar 7, 2022

Deploy Preview for pen-pals ready!

Name Link
🔨 Latest commit 546c2e3
🔍 Latest deploy log https://app.netlify.com/sites/pen-pals/deploys/627de6c89a5be60009a7b33c
😎 Deploy Preview https://deploy-preview-20--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.

@8BitRobot 8BitRobot self-requested a review March 27, 2022 22:31
Copy link
Contributor

@8BitRobot 8BitRobot left a comment

Choose a reason for hiding this comment

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

Good progress so far! Once you fix these things and add the placeholder for the graph box (and fix any linting errors that crop up), it should be good to go.

<h1> Type the correct numbers into the blanks below! </h1>
<h1> A = (2,0) </h1>
<h1> B = (-2,0) </h1>
<h1> C = (-1,0) </h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

A few small things:

  1. Given the layout of the text in the designs, it makes more sense in my opinion to have the actual "exercises," i.e. the problems they have to solve, in their own div (perhaps with its own ID of level-exercise-prompts or similar?). This would allow you to add the additional spacing between the actual heading of the exercise (the main prompt at the top) and the questions without adding some arbitrary spacing between text items 1 and 2. In addition, it would make it easier to dynamically render exercises, for when we do that in the future.
  2. Generally it's bad practice to overuse headings, especially the h1 element which is reserved for top-level headings and the like. I suggest instead that you use p tags for all of these and style them as needed.
  3. The letters A, B, and C should be italicized; I suggest wrapping them in a span element and giving the span the appropriate styling for italics (I think it's something like "font-style: italic"?).

}

button {
background-color: green;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to use the correct shade of green that's in the colors.scss file! You can see an example of how to use variables from that file by looking at line 9 of this file. Just check the colors.scss file for the variable name and include it according to the syntax from line 9. Also, there's no need for a border on this button as it's all one color; the Figma doesn't include any border either.

button {
background-color: green;
border: 1px solid #e1e4e8;
border-radius: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the comment on colors, this should be 12px, not 10px.

@jpaten jpaten self-requested a review May 9, 2023 22:07
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.

2 participants