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

feat: adding cat painting project #50565

Merged

Conversation

Ksound22
Copy link
Member

Checklist:

This is the project that will replace the Picasso painting project.

@Ksound22 Ksound22 requested a review from a team as a code owner May 31, 2023 11:06
@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label labels May 31, 2023
@codesee-maps
Copy link
Contributor

codesee-maps bot commented May 31, 2023

👀 Review this PR in a CodeSee Review Map

View the CodeSee Map of this change

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@moT01
Copy link
Member

moT01 commented Jun 1, 2023

Is this ready to go or are you still working on it @Ksound22?

@Ksound22
Copy link
Member Author

Ksound22 commented Jun 1, 2023

Is this ready to go or are you still working on it @Ksound22?

Not yet. I created the PR for early feedbacks so I won't write tests for what-nots.

Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

I think it is off to a good start.

Left a few comments 👍

Ksound22 and others added 4 commits June 2, 2023 10:28
Co-authored-by: Jessica Wilkins  <67210629+jdwilkin4@users.noreply.github.com>
Co-authored-by: Naomi Carrigan <nhcarrigan@gmail.com>
@Sembauke Sembauke added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Jun 15, 2023
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Hey, this isn't a proper review (though it does look good!) I just noticed a typo when I glanced at this.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Went through all the steps. The tests are working perfectly fine🚀
I have left some comments where the description needs to match the tests.

Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

I went through all of the steps and tests and I think it looks good.
Left a couple of small comments 👍

Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
Co-authored-by: Zaira <33151350+zairahira@users.noreply.github.com>
Co-authored-by: Jessica Wilkins  <67210629+jdwilkin4@users.noreply.github.com>
@ojeytonwilliams ojeytonwilliams dismissed their stale review June 20, 2023 07:01

Changes were implemented

Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks in the suggestions below, but I have some larger thoughts:

First, and I wish I had caught this when this was added to the curriculum expansion repo... There is a lot of repeated CSS. I feel like we're missing an opportunity to reinforce applying styles to multiple classes at once. It would be a pretty big refactor to change that at this point, but IMHO it's worth it. (It also felt tedious as a learner to be writing the same lines so many times)

Second, the indentation for blank lines in the editable regions are inconsistent. I've had some that started with 4 indents when they should only have 2 - even one with a good 10 or so indents. IMHO, new lines we provide for campers to write their code should either start at the appropriate indent level, or no indentation at all.

@naomi-lgbt naomi-lgbt mentioned this pull request Jun 20, 2023
4 tasks
Ksound22 and others added 2 commits June 21, 2023 20:29
@naomi-lgbt
Copy link
Member

I have fixed the indentation part. How would you suggest I refactor the whole code to reduce the repeated CSS?

It would be a pretty big refactor at this point. Might not be worth doing, but essentially the common properties (e.g. width and height for all six whiskers) should be moved to a separate multi-class selector.

@Ksound22
Copy link
Member Author

I have fixed the indentation part. How would you suggest I refactor the whole code to reduce the repeated CSS?

It would be a pretty big refactor at this point. Might not be worth doing, but essentially the common properties (e.g. width and height for all six whiskers) should be moved to a separate multi-class selector.

This works fine on my end:

* {
  margin: 0;
  padding: 0;
  box-sizing: border-box;
}

body {
  background-color: #c9d2fc;
}

.cat-head {
  position: absolute;
  right: 0;
  left: 0;
  top: 0;
  bottom: 0;
  margin: auto;
  background: linear-gradient(#5e5e5e 85%, #45454f 100%);
  width: 205px;
  height: 180px;
  border: 1px solid #000;
  border-radius: 46%;
}

.left-ear {
  position: absolute;
  top: -26px;
  left: -31px;
  z-index: 1;
  transform: rotate(-45deg);
  border-top-left-radius: 90px;
  border-top-right-radius: 10px;
  border-left: 35px solid transparent;
  border-right: 35px solid transparent;
  border-bottom: 70px solid #5e5e5e;
}

.right-ear {
  position: absolute;
  top: -26px;
  left: 163px;
  z-index: 1;
  transform: rotate(45deg);
  border-top-left-radius: 90px;
  border-top-right-radius: 10px;
  border-left: 35px solid transparent;
  border-right: 35px solid transparent;
  border-bottom: 70px solid #5e5e5e;
}

.cat-left-inner-ear {
  position: absolute;
  top: 22px;
  left: -20px;
  border-top-left-radius: 90px;
  border-top-right-radius: 10px;
  border-bottom-right-radius: 40%;
  border-bottom-left-radius: 40%;
  border-left: 20px solid transparent;
  border-right: 20px solid transparent;
  border-bottom: 40px solid #3b3b4f;
}

.cat-right-inner-ear {
  position: absolute;
  top: 22px;
  left: -20px;
  border-top-left-radius: 90px;
  border-top-right-radius: 10px;
  border-bottom-right-radius: 40%;
  border-bottom-left-radius: 40%;
  border-left: 20px solid transparent;
  border-right: 20px solid transparent;
  border-bottom: 40px solid #3b3b4f;
}

.cat-left-eye {
  position: absolute;
  top: 54px;
  left: 39px;
  transform: rotate(25deg);
  width: 30px;
  height: 40px;
  background-color: #000;
  border-radius: 60%;
}

.cat-right-eye {
  position: absolute;
  top: 54px;
  left: 134px;
  transform: rotate(-25deg);
  width: 30px;
  height: 40px;
  background-color: #000;
  border-radius: 60%;
}

.cat-left-inner-eye {
  position: absolute;
  top: 8px;
  left: 2px;
  transform: rotate(10deg);
  width: 10px;
  height: 20px;
  background-color: #fff;
  border-radius: 60%;
}

.cat-right-inner-eye {
  position: absolute;
  top: 8px;
  left: 18px;
  transform: rotate(-5deg);
  width: 10px;
  height: 20px;
  background-color: #fff;
  border-radius: 60%;
}

.cat-nose {
  position: absolute;
  top: 108px;
  left: 85px;
  border-top-left-radius: 50%;
  border-bottom-right-radius: 50%;
  border-bottom-left-radius: 50%;
  transform: rotate(180deg);
  border-left: 15px solid transparent;
  border-right: 15px solid transparent;
  border-bottom: 20px solid #442c2c;
}

/* New multi-level selector for mouth starts */
.cat-mouth div {
  width: 30px;
  height: 50px;
  border: solid 2px #000;
  border-radius: 190%/190px 150px 0 0;
  border-color: black transparent transparent transparent;
}
/* New multi-level selector for mouth ends */

.cat-mouth-line-left {
  position: absolute;
  top: 88px;
  left: 74px;
  transform: rotate(170deg);
}

.cat-mouth-line-right {
  position: absolute;
  top: 88px;
  left: 91px;
  transform: rotate(165deg);
}

/* New multi-level selector for whiskers starts */
.cat-whiskers-left div {
  width: 40px;
  height: 1px;
  background-color: #000;
}

.cat-whiskers-right div {
  width: 40px;
  height: 1px;
  background-color: #000;
}
/* New multi-level selector ends */

.cat-whisker-left-top {
  position: absolute;
  top: 120px;
  left: 52px;
  transform: rotate(10deg);
}

.cat-whisker-left-middle {
  position: absolute;
  top: 127px;
  left: 52px;
  transform: rotate(3deg);
}

.cat-whisker-left-bottom {
  position: absolute;
  top: 134px;
  left: 52px;
  transform: rotate(-3deg);
}

/* Right */
.cat-whisker-right-top {
  position: absolute;
  top: 120px;
  left: 109px;
  transform: rotate(-10deg);
}

.cat-whisker-right-middle {
  position: absolute;
  top: 127px;
  left: 109px;
  transform: rotate(-3deg);
}

.cat-whisker-right-bottom {
  position: absolute;
  top: 134px;
  left: 109px;
  transform: rotate(3deg);
}

Apart from the whiskers, I also moved common selectors of the mouth to a multi-level selector. Is that enough?

@Ksound22
Copy link
Member Author

Ksound22 commented Jul 4, 2023

I have fixed the indentation part. How would you suggest I refactor the whole code to reduce the repeated CSS?

It would be a pretty big refactor at this point. Might not be worth doing, but essentially the common properties (e.g. width and height for all six whiskers) should be moved to a separate multi-class selector.

@naomi-lgbt could you please take another look now? I have moved those properties into some multi-level selectors.

Copy link
Contributor

@Dario-DC Dario-DC left a comment

Choose a reason for hiding this comment

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

I went through all the steps and left a few comments. It looks nice.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Reviewed till step 20.
A quick question- are we checking the semicolons in CSS?

@Ksound22
Copy link
Member Author

Reviewed till step 20. A quick question- are we checking the semicolons in CSS?

No we are not.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Reviewed till step 30.

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

@Ksound22 Reviewed all the steps, just left a few comments.

I also went through issue #50757 , seems like we can skip the checks for closing tags for now.

Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

Hey @Ksound22 !

I think the project looks good.
I just wanted to double check on the open conversations and if they are resolved.

If all of those conversations are resolved and the changes have been applied then I am happy to approve 👍

Co-authored-by: Zaira <33151350+zairahira@users.noreply.github.com>
Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

Hey Kolade, thanks for all of your hard work on this! It's looking great.

I've added a couple of formatting nitpicks - once those are resolved, happy to approve + merge.

Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

Nice work on this Kolade! 💜

@naomi-lgbt naomi-lgbt removed the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Jul 24, 2023
@naomi-lgbt naomi-lgbt changed the base branch from main to naomi/picasso-replacement August 2, 2023 15:06
@naomi-lgbt naomi-lgbt merged commit 55097be into freeCodeCamp:naomi/picasso-replacement Aug 2, 2023
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front 3 2023 platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet