curriculum icon indicating copy to clipboard operation
curriculum copied to clipboard

Bug - Flexbox Growing and Shrinking lesson needs a rewrite

Open aziham opened this issue 3 years ago โ€ข 21 comments

Complete the following REQUIRED checkboxes:

  • [x] I have thoroughly read and understand The Odin Project Contributing Guide
  • [x] The title of this issue follows the Bug - location of bug: brief description of bug format, e.g. Bug - React section: Knowledge Checks don't link to resource

The following checkbox is OPTIONAL:

  • [x] I would like to be assigned this issue to work on it

EDIT - Maintainer Team - Daniel

Acceptance Criteria:

  • [X] Replace current screenshot at start of lesson with an example of flex: 1 and a description of what values that equates to
  • [X] Add a new section that explains flex: auto and the values that it equates to
  • [X] Add a new Assignment section (following the Layout Style Guide) that includes resources that could cover flex shorthand, flex-grow, flex-shrink and flex-basis. See some possible options here:
    • https://www.w3.org/TR/css-flexbox-1/#flex-common
    • https://css-tricks.com/almanac/properties/f/flex/
    • https://developer.mozilla.org/en-US/docs/Web/CSS/flex
  • [ ] Add at least 1 more Knowledge Check to help cover the new lesson content

1. Description of the Bug:

According to https://www.w3.org/TR/css-flexbox-1/#flex-common

flex: initial Equivalent to flex: 0 1 auto. (This is the initial value.) Sizes the item based on the width/height properties. (If the itemโ€™s main size property computes to auto, this will size the flex item based on its contents.) Makes the flex item inflexible when there is positive free space, but allows it to shrink to its minimum size when there is insufficient space. The alignment abilities or auto margins can be used to align flex items along the main axis.

The flex-basis initial value is 'auto' not '0%'.

The flexbox growing and shrinking lesson stated:

The default value of the flex property is shown in the above screenshot: flex-grow: 0, flex-shrink: 1, flex-basis: 0%.

Which are not the initial values of the flex property neither the default values for the shorthand flex: 1.

My closest guess is that the statement above should be:

The default value of the flex: 1 property is shown in the above screenshot: flex-grow: 1, flex-shrink: 1, flex-basis: 0%.

Which is gonna lead to a rewrite of the whole section.

I opened a PR #24175 and closed it afterwards due to the confusion.

2. How To Reproduce:

  1. Visit: https://www.theodinproject.com/lessons/foundations-growing-and-shrinking

aziham avatar May 30 '22 20:05 aziham

@Hamza-Aziane could you share any document reference for supporting this fact "The flex-basis initial value is 'auto' not '0%'." thanks :)

bappyasif avatar Jun 05 '22 19:06 bappyasif

@Hamza-Aziane could you share any document reference for supporting this fact "The flex-basis initial value is 'auto' not '0%'." thanks :)

I've already quoted w3c (the main international standards organization for the World Wide Web) documentation in the description of the bug. Please read the issue's description again. Thank you.

aziham avatar Jun 05 '22 22:06 aziham

@Hamza-Aziane thanks for pointing that out, i was busy reading through description but missed that 'w3c' citation somehow!!

i also looked into mdn articles, and they also cited same in this article https://developer.mozilla.org/en-US/docs/Web/CSS/flex#formal_definition

on an another note, perhaps that code snippet of flex: 0 1 0% was to tally pointing at "example" at hand!! in that case perhaps we need to adjust that as well

happy contributing :)

bappyasif avatar Jun 06 '22 13:06 bappyasif

on an another note, perhaps that code snippet of flex: 0 1 0% was to tally pointing at "example" at hand!! in that case perhaps we need to adjust that as well

If it's somehow pointing to the example at hand i.e. flex: 1 then it should be flex: 1 1 0%.

I've already mentioned that in the issue's description:

The flexbox growing and shrinking lesson stated:

The default value of the flex property is shown in the above screenshot: flex-grow: 0, flex-shrink: 1, flex-basis: 0%.

Which are not the initial values of the flex property neither the default values for the shorthand flex: 1.

My closest guess is that the statement above should be:

The default value of the flex: 1 property is shown in the above screenshot: flex-grow: 1, flex-shrink: 1, flex-basis: 0%.

The problem is both cases will lead to a rewrite of the whole section.

aziham avatar Jun 06 '22 15:06 aziham

seems like a due pr to me! lets see what maintainers has to say about this :)

bappyasif avatar Jun 06 '22 16:06 bappyasif

seems like a due pr to me! lets see what maintainers has to say about this :)

I opened a PR and closed it afterwards because I didn't know what the writer of that section was referring to with that flex example.

@bappyasif You are right, this issue needs a maintainer to clear the confusion, then decide how it should be fixed.

aziham avatar Jun 07 '22 12:06 aziham

@Hamza-Aziane Definitely seems like a fix is in order. I have only had a quick glance through the lesson. Do you think the actual examples are incorrect, or is it just a matter of updating a handful of values throughout the lesson?

ChargrilledChook avatar Jun 18 '22 15:06 ChargrilledChook

The confusion here might be that there is no "default flex value". It's not something you can write flex: ; in your css and have it work. You need to use: element { flex: auto } OR element { flex: none } OR specifiy the grow, shrink, and basis values yourself.

With grow, shrink, and basis, if you omit any of them, then they revert to their default values of 1 1 0 respectively.

In the below screenshot, you can see the initial definition of the flex shorthand:

image

Where it specifies Initial: none, that actually indicates what the value of flex: none should be, which is 0 1 auto.

Further down we have: image snip image snip image

There is then the section for flex with keyword values: image

My suggestion, we pick one of the keyword values and use that as a baseline for what "default" flex means throughout the lesson.

xandora avatar Jun 18 '22 19:06 xandora

Thanks @Hamza-Aziane ๐Ÿ‘‹

This issue has come up enough times in PRs and on Discord that I believe it's worth addressing. Some good points already made from @xandora!

In my opinion, the confusion in our lesson stems from the screenshot and the first line following the screenshot:

Screen Shot 2022-06-18 at 9 13 56 PM

Additionally the Knowledge Check link brings learners back up to this screenshot to answer the question: "What are the 3 values defined in the shorthand flex property (e.g. flex: 1 1 auto)?"

The lesson itself is supposed to explain flex: 1 per the opening line: "Letโ€™s look a little closer at what actually happened when you put flex: 1 on those flex items in the last lesson." In the preceding lesson, learners added a flex: 1 value in the embedded CodePens.

flex: 1 is a shorthand property with a one-value syntax. flex: 1 defines the following values:

flex-grow: 1 flex-shrink: 1 flex-basis: 0

If anything, I think the screenshot could be focused on just showing flex: 1 and then detailing what that stands for.

What makes the flex shorthand value more confusing is the default or initial values for flex-grow, flex-shrink, and flex-basis.

The lesson covers this a bit throughout and here is a screen of the full flex-basis section:

Screen Shot 2022-06-18 at 9 43 32 PM

What I'm most concerned about is if this lesson's scope is too much trying to get into the details of the flex shorthand and its defaults while also explaining the purposes of the flex-grow, flex-shrink and flex-basis values. It feels like a lot when someone is just learning flexbox!

Really interested to hear any other thoughts on this lesson and how it can be clarified for learners just getting started with flexbox.

dm-murphy avatar Jun 19 '22 01:06 dm-murphy

Do you think the actual examples are incorrect, or is it just a matter of updating a handful of values throughout the lesson?

@ChargrilledChook I think that the codepen examples are correct.

The lesson itself is supposed to explain flex: 1 per the opening line: "Letโ€™s look a little closer at what actually happened when you put flex: 1 on those flex items in the last lesson." In the preceding lesson, learners added a flex: 1 value in the embedded CodePens.

@dm-murphy I totally agree with you. I think this lesson should be modified to only cover what the reader expects to be explained i.e: flex:1

image

My suggestion is the same as @xandora. Taking the above screenshot as a baseline and rewrite the lesson so the learners doesn't get confused.

Thank you guys for all the time and effort you put into this brilliant curriculum. When I recommend it to someone, I get asked what is the special thing about it? I tell them that TOP's curriculum is a project based curriculum, so if you are someone who has been suffering from tutorial hell it's time to stop using the training wheels.

aziham avatar Jun 22 '22 23:06 aziham

I agree with @Hamza-Aziane, the lesson should be modified to teach the reader what has been shown in the flex example i.e, flex: 1. Besides, the following MDN article about the flex can be used as reference in the course or maybe even in the assignment. Flex property is one of most confusing properties, linking the learner to an appropriate source would be good.

I had a related discussion with daniel on the discord server here

ManorSailor avatar Jun 25 '22 04:06 ManorSailor

@Hamza-Aziane and @ManorSailor Just want you both to know this issue has not been forgotten about and we really appreciate the input! ๐Ÿ™

@Hamza-Aziane Are you still willing to take on this issue and make a pull request? It is likely to take a number of iterations to work through this lesson.

We will also need to update this issue with the criteria of what exactly we want to address in reworking this lesson. I will put together some thoughts on that but in the meantime let me know if you are still interested in being assigned, thanks!

dm-murphy avatar Jun 29 '22 00:06 dm-murphy

What I'm most concerned about is if this lesson's scope is too much trying to get into the details of the flex shorthand and its defaults while also explaining the purposes of the flex-grow, flex-shrink, and flex-basis values. It feels like a lot when someone is just learning flexbox!

Hey just my two cents but, I feel like this is a familiar feeling amongst all learners (even I felt it going through it) and I think we could split all the details about flex-grow, flex-basis, and flex initial properties into a separate lesson or maybe an assignment.

The lesson's central message is to explain what flex: 1 is and we should only explain growth shrink and basis properties to that extent, Making the reader understand what flex: 1 1 0 is without going too in-depth about specifics about flex-grow: 2, etc. as this is where in my opinion it gets too overwhelming.

Would love to contribute to this rewrite if it's still in need of help!

aaronlrv avatar Jul 05 '22 05:07 aaronlrv

@dm-murphy It is a pleasure to work on this issue in order to improve TOP's curriculum.

aziham avatar Jul 05 '22 16:07 aziham

@dm-murphy It is a pleasure to work on this issue in order to improve TOP's curriculum.

Thank you @Hamza-Aziane! I have assigned you to this issue.

Please note I have updated the issue above to include an Acceptance Criteria section with 4 items. That should be enough to get us started with reworking this lesson and we can take things from there. All the criteria should be separate enough that they could be covered by multiple PRs. But I would suggest starting with the first one on the screenshot as that is causing the most trouble in the lesson.

@aaronlrv I am going to assign you as well to help assist with this one. Could you focus on the 3rd criteria item of adding in an Assignment Section and choosing which resources you think might be best in covering these topics? Some options were included in the criteria section but if you find a better resource feel free to suggest that instead.

Once we have these steps all complete we can see if the lesson needs more rewriting. Can you both also ping me when opening a PR on this issue and mention the issue number in the template, but don't use the "Closes" tag since this will require multiple PRs to complete everything.

If you have any questions at all please just ask and thanks! ๐Ÿ™‚

dm-murphy avatar Jul 06 '22 20:07 dm-murphy

Hey! @dm-murphy Heres the PR regarding the assignment section as mentioned. Please do check and let me know!

PR Number #24386

Edit: I've just seen that my commits for a previous PR #24379 regarding style content got bundled up with this one, My apologies. Will try to remove these commits from this assignment specific PR asap.

aaronlrv avatar Jul 07 '22 18:07 aaronlrv

Hey! @dm-murphy , I was wondering if I could take on the flex:auto section task now as I've finished with the assigmment section. Please do let me know!

aaronlrv avatar Jul 24 '22 10:07 aaronlrv

Hey! @dm-murphy , I was wondering if I could take on the flex:auto section task now as I've finished with the assigmment section. Please do let me know!

Hi @aaronlrv ๐Ÿ‘‹ Go for it! Thanks!

dm-murphy avatar Jul 24 '22 16:07 dm-murphy

Hey @dm-murphy! Just wondering if I can finish up this issue and take on the remaining 2 tasks that need to be done (knowledge check, and the image)

aaronlrv avatar Jul 31 '22 08:07 aaronlrv

Hey @dm-murphy! Just wondering if I can finish up this issue and take on the remaining 2 tasks that need to be done (knowledge check, and the image)

Thanks for asking @aaronlrv! Since aziham is already working on the image task, how about you get started on the Knowledge Check task? Please be sure to follow the Layout Style Guide for how to properly format a Knowledge Check link. Thanks again for all your help on this issue ๐Ÿ™‚

dm-murphy avatar Jul 31 '22 13:07 dm-murphy

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

github-actions[bot] avatar Sep 02 '22 02:09 github-actions[bot]

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

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