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

Foundations/Growing and Shrinking/Provide descriptive link text #27784

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

Conversation

Rllopez0410
Copy link
Contributor

Because

Links in the lesson should be sufficiently descriptive to improve accessibility. As per the link text, it should both concisely describe its purpose.

This PR

  • Changed the text on line 81.
  • Change the text on line 96.

Issue

Related to #27681.

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added the Content: Foundations Involves the Foundations content label Apr 12, 2024
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Thanks, @Rllopez0410. Mind fixing up the lint errors while you're at it? They should be pretty self-explanatory.

@MaoShizhong MaoShizhong self-assigned this Apr 12, 2024
@Rllopez0410
Copy link
Contributor Author

Hello @MaoShizhong, I'm not sure if I'm on going about fixing these linting errors correctly; can you see if this is correct when you get the chance?

@MaoShizhong
Copy link
Contributor

MaoShizhong commented Apr 16, 2024

@Rllopez0410 The most recent commit (5b347e6) ended up removing a bunch of ` that need to be put back, as the inline code blocks were needed for all of them, do differentiate the code values/keywords from normal words in the sentence.

As for the lint errors in general, you can view the errors in the action details:
image

Or in the Files changed tab at the top of the PR:
image

The error messages tell you what needs to be done. For example, on line 51, it expects a blank line both before and after it, but it did not find a blank line after it, so you'll need to insert one there. Same with line 55 (but insert before for that one).

Or on line 64, the heading ends with : which the error says is not allowed, so removing it resolves that. The same applies to the ... on line 72.

I will add a review comment below for how to resolve the error on line 67, as that one needs to use note boxes for those two subsections instead of block quotes.

@MaoShizhong
Copy link
Contributor

Lines 64-70 currently look like this:

> #### Important note about flex-basis:
>
> There is a difference between the default value of `flex-basis` and the way the `flex` shorthand defines it if no `flex-basis` is given. The actual default value for `flex-basis` is `auto`, but when you specify `flex: 1` on an element, it interprets that as `flex: 1 1 0`. If you want to _only_ adjust an item's `flex-grow` you can do so directly, without the shorthand. Or you can be more verbose and use the full 3 value shorthand `flex: 1 1 auto`, which is also equivalent to using `flex: auto`.

> #### What is flex: auto?
>
> If you noticed, we mentioned a new flex shorthand `flex: auto` in the previous note. However we didn't fully introduce it. `flex: auto` is one of the shorthands of flex. When `auto` is defined as a flex keyword it is equivalent to the values of `flex-grow: 1`, `flex-shrink: 1` and `flex-basis: auto` or to `flex: 1 1 auto` using the flex shorthand. Note that `flex: auto` is not the default value when using the flex shorthand despite the name being "auto" which may be slightly confusing at first. You will encounter and learn more about `flex: auto` and its potential use-cases when reading through the assignment section.

Could you replace those lines with the following?

<div class="lesson-note" markdown="1">

#### Important note about flex-basis

There is a difference between the default value of flex-basis and the way the flex shorthand defines it if no flex-basis is given. The actual default value for flex-basis is auto, but when you specify flex: 1 on an element, it interprets that as flex: 1 1 0. If you want to only adjust an item's flex-grow, you can do so directly, without the shorthand. Or you can be more verbose and use the full 3-value shorthand flex: 1 1 auto, which is also equivalent to using flex: auto.

</div>

#### What is flex: auto?

If you noticed, we mentioned a new flex shorthand `flex: auto` in the previous note. However we didn't fully introduce it. `flex: auto` is one of the shorthands of flex. When `auto` is defined as a flex keyword it is equivalent to the values of `flex-grow: 1`, `flex-shrink: 1` and `flex-basis: auto` or to `flex: 1 1 auto` using the flex shorthand. Note that `flex: auto` is not the default value when using the flex shorthand despite the name being "auto" which may be slightly confusing at first. You will encounter and learn more about `flex: auto` and its potential use-cases when reading through the assignment section.

@MaoShizhong
Copy link
Contributor

@Rllopez0410 Please see above regarding how to see the linting errors and how to resolve them, as well as the fix require for the blockquote error on line 67, as it'd be better to not use blockquotes for those bits anyway.

It might be a bit awkward trying to transfer this to your local machine to work on right now, but for future PRs, you'll likely find it far easier to do lint checks/fixes and make bigger changes to multiple files (in the same branch and PR!) if you work locally.

For reference, our general contributing guide has instructions for setting up your local fork (and keeping your main branch up to date), and creating branches off main where you'll do your actual work.
The curriculum repo-specific contributing guide has instructions for how to run lint checks and fixes (for some of the rules) locally.

@Rllopez0410
Copy link
Contributor Author

@MaoShizhong, understood. I'll look into the docs before my next PR. I appreciate you guiding me to the appropriate resources! I'll finish this PR ASAP.

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Thanks for sorting out the emphasis markers. There are still some blank line lint errors to resolve, some of which look to be caused by lines looking blank but actually containing some number of spaces. There are also the default section content for the knowledge check and additional resources to fix, which would just be replacing the opening text to what the error says it expects instead.

You can view any linting errors to go in either the Files changed tab, or via the action details - shown in one of the images above.

When you push commits, the action will rerun and notify you if it fails. If it passes, you'll see a big green tick here saying all checks passed.

@MaoShizhong
Copy link
Contributor

@Rllopez0410 Just checking in if you've seen the above - no rush at all (genuinely) if you've been busy with other things. There's also a new.merge conflict to resolve which should be straightforward (get rid of line starting with * and keep your one starting with -)

@Rllopez0410
Copy link
Contributor Author

Hey @MaoShizhong I've just seen the new merge conflict; I'll be able to get back to working on this PR tomorrow. I appreciate the patience you've had with me during this contribution 🙏.

@MaoShizhong
Copy link
Contributor

No problem at all! Just a courtesy check in because I know recently a few review change requests haven't sent out notifications (has happened to me too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: Foundations Involves the Foundations content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants