Skip to content

Conversation

Zekumoru
Copy link
Contributor

Third and fourth elements share the same declaration of font-size.

Complete the following REQUIRED checkboxes:

  • 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. 01-flex-center: Update self check

Complete the following checkbox ONLY IF it is applicable to your PR. You can complete it later if it is not currently applicable:

  • I have ensured that the TOP solution files match the Desired Outcome image

1. Because:

The third and fourth element share the same declaration for font-size of 24px and since the formerly class 'oddly-cool' already declared it, I removed the same declaration from the 'fourth' id and use the new renamed class 'adjust-font-size' of 'oddly-cool' to the fourth element instead. Also, this refreshes the idea that you can put id and class in the same element.

2. This PR:

  • rename 'oddly-cool' class to 'adjust-font-size'
  • remove 24px font size declaration from the id 'four'
  • append the renamed class in the class attribute of the fourth element

3. Additional Information:

Third and fourth elements share the same declaration for font-size.
@Zekumoru Zekumoru force-pushed the fix_exercise_solution branch from 574f345 to 4634b5f Compare June 29, 2022 02:43
@dm-murphy dm-murphy added the Status: Needs Review This issue/PR needs an initial or additional review label Jul 7, 2022
@dm-murphy dm-murphy requested a review from thatblindgeye July 7, 2022 18:07
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks like a good change! Left one comment below

@@ -14,6 +14,5 @@

#four {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is still relatively early and users may not have had a chance to gain an understanding of reducing code duplication, I think it would be worth adding a comment above the #four block along the lines of "Instead of applying the adjust-font-size class to this element in the HTML file, you could have also simply added a rule to the #four selector to set the font size. Reusing the class on this element helps reduce duplicate code."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree as well because it isn't apparent that the class adjust-font-size is meant to reduce code duplication and new users will miss it out.

@thatblindgeye thatblindgeye added Status: Awaiting Response Waiting for a response from the contributor and removed Status: Needs Review This issue/PR needs an initial or additional review labels Jul 9, 2022
@Zekumoru Zekumoru requested a review from thatblindgeye July 10, 2022 09:40
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review, but thanks for making this update!

@thatblindgeye thatblindgeye merged commit 8bea2cf into TheOdinProject:main Nov 19, 2022
Oussama5379 added a commit to Oussama5379/css-exercises that referenced this pull request Feb 1, 2025
…tion

02-class-id-selectors: Change solution
repo-jr added a commit to repo-jr/css-exercises that referenced this pull request Aug 29, 2025
…tion

02-class-id-selectors: Change solution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response Waiting for a response from the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants