curriculum icon indicating copy to clipboard operation
curriculum copied to clipboard

Introduction to Flexbox: Introduces concepts not covered in lesson

Open 749cakeee opened this issue 1 year ago • 1 comments

Checks

Describe your suggestion

Summary

As stated in #26137, there are some concepts like gap that aren't referred to or explained anywhere in the current lesson.

Since lessons in the Odin Project do highlight when a concept is introduced that is not in the current lesson, not finding an explanation for the gap confused me.

Description

image

The above image looked off, so I tried to recreate it only to find out that the given CSS properties don't give the same output.

I then tried to play around with the margins and reached the conclusion that I had to add margins to the item in the middle (and only this item) to get the given output.

It was after this that I looked up the issues here and found #26137 which mentioned the gap property.

This explained everything.

Hitherto, if there is something that is going to be explained in the upcoming lessons yet is somehow mentioned or referred to in the current lesson, there is always a call-out of some sorts. This sets some (good) expectations.

You can see an example below from this lesson:

image

Suggestions

To improve the learning experience, we should consider implementing either of the following suggestions:

1. Edit the image

We can edit the CSS in the image to include the gap property. Keen learners can then look this up or infer that this is what adds the space between the items when there is no margin defined.

2. [Preferred] Add a call-out

Alternatively, we can add a small call-out below the image that mentions the gap property and nudges learners to the issues section as so:

"...remember, if you don't understand something, feel free to Google or look up issues here..."

Path

Foundations

Lesson Url

https://www.theodinproject.com/lessons/foundations-introduction-to-flexbox

(Optional) Discord Name

749cake

(Optional) Additional Comments

No response

749cakeee avatar Feb 17 '24 06:02 749cakeee

Gap is a property that is covered in a later lesson. So there is no need to ask learners to try to search it up, when it's already explained at a later point.

Another alternative is to add a note that these images have gap property that will be covered in a later lesson and learner's shouldn't worry about it. But I don't think this is a good idea, why not just fix the images instead?

So lets fix the images instead. These two images should be changed so that they don't include the gap property.

This image doesn't need to be changed. It doesn't use a code example, just demonstrates what is possible. Perhaps adding a note akin to 'you'll be able to make this type of layout by the end of this section' or something like that. But not necessary.

So what do you think about this?

Just my 2 cents

nik-rev avatar Feb 19 '24 02:02 nik-rev

Thanks @749cakeee for opening the issue and @nikitarevenco for the input.

Just investigated this and it looks like the only issue was the incorrect CSS properties were applied in the fix PR.

The following will produce the correct display in the first image without the gap property:

.container {
    background-color: aqua;
    padding: 12px;
    border: 4px solid blue;
    display: flex;
}

.item {
    background-color: peachpuff;
    border: 4px solid salmon;
    height: 48px;
    flex: 1;
    margin: 4px;
}

assuming an intuitive HTML structure of

<div class="container">
    <div class="item"></div>
    <div class="item"></div>
    <div class="item"></div>
</div>

Essentially, replacing gap with margin had margin put on the incorrect selector. gap goes on the flex container whilst margins go on the flex items - in the PR, margin was put on the flex container instead.

What I've done to replicate the image is the following:

  • Replace .item's padding for margin (keeping 4px to better reflect the image's scale). The flex items do not need a padding in this scenario.
  • Removed the margin property entirely from .container as its sole purpose was to "replace gap" but should've gone on .item instead.
  • Changed .container's padding to 12px just so the scaling better reflects the image.

I'd be very happy to have the image with the CSS fixed to show the CSS I've used above @749cakeee. I can assign you if you're happy to make implement this change. Changes to images in this repo needs to be done in 2 separate PRs as per the wiki instructions on adding images to the curriculum repo. i.e. one PR to replace the image in question, then after that's merged, a second PR to have the lesson file use the appropriate Statically CDN link (and delete the old image file).

mao-sz avatar Mar 05 '24 13:03 mao-sz

This issue is stale because it has had no activity for the last 30 days.

github-actions[bot] avatar Apr 05 '24 01:04 github-actions[bot]

@749cakeee Are you still interested in working on this issue?

CouchofTomato avatar Apr 05 '24 14:04 CouchofTomato

If no one is working on this I am happy to take it :)

yuliana-r avatar Apr 09 '24 14:04 yuliana-r

It doesn't look like the issue author is active so I'll assign this to you @yuliana-r. The changes required are in my comment above, as are the instructions for how to handle adding images to the curriculum, meaning this change will be a 2-PR process.

Let me know if you have any questions.

mao-sz avatar Apr 09 '24 16:04 mao-sz

@CouchofTomato, sorry, just saw this!

Yeah, I'm still interested in working on this issue 😄

@yuliana-r, since this is a 2-PR process, would you like to split the work? 😋

749cakeee avatar Apr 09 '24 16:04 749cakeee

@749cakeee heyy happy for you to take over if you want to work on both, otherwise I don't mind taking one PR! :)

yuliana-r avatar Apr 09 '24 16:04 yuliana-r

Guess I was a little too hasty in the assignment. It'll be easier for one person to do both PRs since they're back to back and not simultaneous.

If you're okay with that, @yuliana-r then I'll swap assignment to @749cakeee for this one.

mao-sz avatar Apr 09 '24 16:04 mao-sz

Yes absolutely, enjoy @749cakeee ! :)

yuliana-r avatar Apr 09 '24 16:04 yuliana-r

Thanks, @yuliana-r!

Perhaps we can collab on another PR 😋

@MaoShizhong, how does this look? 🤔

Just wanted to run it by you before I go ahead ⬇️

03

749cakeee avatar Apr 09 '24 16:04 749cakeee

@749cakeee :ok_hand: Just realised the original .container is lightblue and not aqua, no idea why my suggestion ended up with aqua XD We can keep aqua - just a funny observation.

If you go ahead with the first PR to add that image file to the repo, then once that's merged, you can make the 2nd PR to swap the image link in the lesson and delete the old image file from the repo.

mao-sz avatar Apr 09 '24 16:04 mao-sz

@MaoShizhong, not sure if you got a notification so just tagging you here!

749cakeee avatar Apr 09 '24 17:04 749cakeee

@749cakeee Just checking in as the first step (replacing the image file) has been completed and merged. You're ready to do the second PR where you replace the image link in the lesson with the new one generated by Statically as per https://github.com/TheOdinProject/curriculum/wiki/Adding-Images-to-the-Curriculum

mao-sz avatar Apr 18 '24 22:04 mao-sz

Hey @MaoShizhong, just created the second PR! 👉 https://github.com/TheOdinProject/curriculum/pull/27817

749cakeee avatar Apr 19 '24 11:04 749cakeee