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

feat(curriculum): add magazine layout lab #57174

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

Conversation

larymak
Copy link
Contributor

@larymak larymak commented Nov 15, 2024

Checklist:

Closes #XXXXX

@github-actions github-actions bot added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: i18n language translation/internationalization. Often combined with language type label labels Nov 15, 2024
@larymak larymak marked this pull request as ready for review November 29, 2024 10:04
@larymak larymak requested a review from a team as a code owner November 29, 2024 10:04
Copy link
Contributor

@Dario-DC Dario-DC left a comment

Choose a reason for hiding this comment

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

I had a brief look and remembered that we have a workshop and a lab in this module with almost the same title. We briefly discussed about modifying this lab in some way. See this conversation: https://chat.google.com/room/AAAAVgjQ1iA/TYbRFIcNm5E/TYbRFIcNm5E?cls=10

Do we still want to do that? @jdwilkin4 I hope there's time.

"In this lab, you will design a magazine layout using CSS Grid, including concepts like grid rows and grid columns."
]
},
"harp": { "title": "115", "intro": [] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"harp": { "title": "115", "intro": [] },

block 115 is already there

demoType: onClick
---

# --description--
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# --description--
# --description--

title: Design a Magazine Layout Using CSS Grid
challengeType: 14
dashedName: design-a-magazine-layout-using-css-grid
# demoType can either be 'onClick' or 'onLoad'. If the project or lab doesn't have a preview, delete the property
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# demoType can either be 'onClick' or 'onLoad'. If the project or lab doesn't have a preview, delete the property

@@ -2447,7 +2447,13 @@
"In this workshop, you'll build a magazine article. You'll practice how to use CSS Grid, including concepts like grid rows and grid columns."
]
},
"ogko": { "title": "114", "intro": [] },
"lab-magazine-layout": {
"title": "Design a Magazine Layout Using CSS Grid",
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming convention we are using is: lab-x -> build-x or design-x (not build-x-by-using-y).

const featureArticle = document.querySelector('section.feature-article');
const smallArticle1 = document.querySelector('section.small-article1');
const smallArticle2 = document.querySelector('section.small-article2');
const coverImage = document.querySelector('section.cover-image');
Copy link
Contributor

Choose a reason for hiding this comment

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

missing asserts in this test

assert.exists(document.querySelector('main'));
```

You should have a `.magazine-cover` class.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not checking that the main element has the requested class

You should have a `header` element for the title.

```js
assert.exists(document.querySelector('header'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should check that the header is a child of the main element

assert.exists(document.querySelector('header'));
```

You should have a `section` element for each article and the cover image, with classes `feature-article`, `small-article1`, `small-article2`, and `cover-image` respectively.
Copy link
Contributor

Choose a reason for hiding this comment

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

Users stories don't specify exactly what you need to do with articles and classes. It should be clarified from the beginning.

@jdwilkin4
Copy link
Contributor

@Dario-DC

Thanks for bringing that up. I forgot about that conversation.

Here is what we should do.

For the MVP launch, let's just leave this as is.
Then after launch, refactor it to a newspaper lab. Here is the issue for that
freeCodeCamp/CurriculumExpansion#670

This isn't the first lab they will encounter in the curriculum. They have to get through all of HTML and most of CSS before they reach this part. Even if some want to randomly jump to the grid that would be tiny minority that it is not much of a concern.

As long as the refactor happens in January of next year, then most campers will only ever see the revised newspaper version anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants