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: add background #81

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: add background #81

wants to merge 4 commits into from

Conversation

NunonuN
Copy link
Collaborator

@NunonuN NunonuN commented Nov 13, 2019

Add background scenario and update some elements of foreground scenario.

Background:

  • Add left mountain;
  • Add right mountain;
  • Add city skyline;

Sky:

  • Change position of star behind tree;

Foreground:

  • Add hear to dog;
  • Change tail of dog;
  • Change size of telescope;
  • Change position and size of hill.

Add background scenario and update some elements of foreground scenario.

Background:

 - Add left mountain;

 - Add right mountain;

 - Add city skyline;

Sky:

 - Change position of star behind tree;

Foreground:

 - Add hear to dog;

 - Change tail of dog;

 - Change size of telescope;

 - Change position and size of hill.
Copy link
Member

@juancarlosfarah juancarlosfarah left a comment

Choose a reason for hiding this comment

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

@NunonuN, it's looking good in general, but there are some Path tags in the SVGs that are empty, so I think that we can remove them.

src/components/svgs/MountainLeft.js Outdated Show resolved Hide resolved
src/components/svgs/Hill.js Outdated Show resolved Hide resolved
src/components/svgs/MountainRight.js Outdated Show resolved Hide resolved
src/components/svgs/City.js Outdated Show resolved Hide resolved
@NunonuN NunonuN changed the title feat: add backgroung feat: add background Nov 14, 2019
@NunonuN
Copy link
Collaborator Author

NunonuN commented Nov 14, 2019

Yes, we can remove all those empty paths. In fact, I already did it. I left them yesterday by mistake. Sorry about that.
I sent you a message in Slack (need your help with git).

Commented out and replaced first line of code in following files inside
src/components/svgs/:

- City.js
- Dog.js
- MountainLeft.js
- MountainRight.js
- TreeLeft.js
- TreeRigh.js

Merge branch 'master' into 26/add-background
Commented out and updated first line of code of the following files
inside the src/components/svgs/ directory:

- City.js
- Dog.js
- MountainLeft.js
- MountainRight.js
- TreeLeft.js
- TreeRight.js
Copy link
Member

@juancarlosfarah juancarlosfarah left a comment

Choose a reason for hiding this comment

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

Hi @NunonuN, this all looks good! Just some very minor comments. Also, could you add a screenshot to the conversation? That way I can see how it looks without building your branch from scratch. Thanks!

@@ -1,20 +1,17 @@
import React, { Fragment } from 'react';
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

@@ -1,18 +1,17 @@
import React, { Fragment } from 'react';
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

@@ -76,7 +76,6 @@ const Hill = ({ x, y, scaleX, scaleY, fill }) => (
scaleX={scaleX}
scaleY={scaleY}
/>
<Path x={x} y={y} data="" fill={fill} scaleX={scaleX} scaleY={scaleY} />
</Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Do you not want to remove <Fragment> here?

@@ -1,19 +1,17 @@
import React, { Fragment } from 'react';
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

@@ -1,19 +1,17 @@
import React, { Fragment } from 'react';
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

@@ -0,0 +1,29 @@
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

@@ -0,0 +1,29 @@
// import React, { Fragment } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this if no longer needed?

Merge branch 'master' into 26/add-background
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants