Introduction to Flexbox: Introduces concepts not covered in lesson
Checks
- [X] This is not a duplicate of an existing issue (please have a look through our open issues list to make sure)
- [X] I have thoroughly read and understand The Odin Project Contributing Guide
- [X] Would you like to work on this issue?
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
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:
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
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
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'spaddingformargin(keeping4pxto better reflect the image's scale). The flex items do not need a padding in this scenario. - Removed the
marginproperty entirely from.containeras its sole purpose was to "replacegap" but should've gone on.iteminstead. - Changed
.container'spaddingto12pxjust 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).
This issue is stale because it has had no activity for the last 30 days.
@749cakeee Are you still interested in working on this issue?
If no one is working on this I am happy to take it :)
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.
@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 heyy happy for you to take over if you want to work on both, otherwise I don't mind taking one PR! :)
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.
Yes absolutely, enjoy @749cakeee ! :)
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 ⬇️
@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.
@MaoShizhong, not sure if you got a notification so just tagging you here!
@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
Hey @MaoShizhong, just created the second PR! 👉 https://github.com/TheOdinProject/curriculum/pull/27817