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

Design Changes - Brush Buttons, Layout, Color #2

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

Conversation

aswathirazak1
Copy link

hey, please review code?

@aswathirazak1 aswathirazak1 changed the title First Pull Request Design Changes - Brush Buttons, Layout, Color Jun 11, 2019
}


Copy link
Owner

Choose a reason for hiding this comment

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

extra blank lines could be removed here


.toolbox__item:hover {
box-shadow: 4px 10px 14px 0px rgba(0,0,0,0.15);
}
Copy link
Owner

Choose a reason for hiding this comment

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

indentation issue. 4 spaces before box-shadow instead of 8. } should be shifted to the beginning of the line

position:absolute;
left:50px;
bottom: 50px;
width:1600px;
Copy link
Owner

Choose a reason for hiding this comment

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

this is too wide. we might have to think about how to make it scale according to the viewport width until some minimum width. narrower than that will qualify for some mobile effort which could be done later

align-items: center;
#key-selection-area {
position:absolute;
left:50px;
Copy link
Owner

Choose a reason for hiding this comment

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

space after :

@@ -3,7 +3,7 @@
body {
font-family: "Benton Sans", "Helvetica Neue", helvetica, arial, sans-serif;
margin: 2em;
background-color: #5c5470;
background-image: linear-gradient(100deg, #8256D6, #5943C8, #3C39BC);
Copy link
Owner

Choose a reason for hiding this comment

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

looks nice!

@@ -117,6 +117,7 @@ var eraser = document.querySelector('#toolbox__eraser')

var tool = 0 // 0 = pen, 1 = eraser


Copy link
Owner

Choose a reason for hiding this comment

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

we should avoid extraneous changes

viz.text('Draw something here', WIDTH / 2 - 150, HEIGHT / 2)
viz.textSize(20)
viz.fill(300)
viz.text('Scribble, quibble, dribble, you know what to do. Hit spacebar when you’re done.', WIDTH / 2 - 300, HEIGHT / 2)
Copy link
Owner

Choose a reason for hiding this comment

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

lol

viz.fill(150)
viz.text('Draw something here', WIDTH / 2 - 150, HEIGHT / 2)
viz.textSize(20)
viz.fill(300)
Copy link
Owner

@dash1291 dash1291 Jun 11, 2019

Choose a reason for hiding this comment

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

fill() takes color as a parameter. 300 means gray value = 300 which is out of bounds (0-255 being the permissible range). check this out - https://p5js.org/learn/color.html

viz.fill(150)
viz.text('Preparing stuff', WIDTH / 2 - 100, HEIGHT / 2)
viz.textSize(20)
viz.fill(300)
Copy link
Owner

Choose a reason for hiding this comment

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

fill() takes color as a parameter. 300 means gray value = 300 which is out of bounds (0-255 being the permissible range). check this out - https://p5js.org/learn/color.html

color: #333;
background: #dbd8e3;
background: #FF90C3;
box-shadow: 4px 10px 14px 0px rgba(168,137,26,0.4);
Copy link
Owner

Choose a reason for hiding this comment

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

spaces after commas :)

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