-
Notifications
You must be signed in to change notification settings - Fork 1
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
Creates new view on popup and overlay #64
base: master
Are you sure you want to change the base?
Conversation
over.css
Outdated
@@ -0,0 +1,73 @@ | |||
*{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, our style convention (and most places) is to add spaces between pretty much everything. It may feel icky at first, but you eventually get used to it and it is usually easier to read quickly. In general, there is no objective universal right style; but the right style is the style the codebase is already using (unless it's obviously objectively terrible, like putting multiple statements on one line). So just copy the style that we've used here; I think it's pretty consistent.
over.css
Outdated
height: 60px; | ||
} | ||
|
||
#top input[type=text]{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this should be:
#top input[type=text] {
over.css
Outdated
@@ -0,0 +1,73 @@ | |||
*{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally try to avoid * selector because it's just too unpredictable. You don't know when the styles you're applying will have unintended effects. Can you put this on body?
over.css
Outdated
*{ | ||
box-sizing: border-box; | ||
} | ||
body{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code
over.css
Outdated
width: 75px; | ||
height: 75px; | ||
text-align: center; | ||
font-size: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally try to use em for font sizes. Read this for more info: https://css-tricks.com/why-ems/
popup.css
Outdated
@@ -0,0 +1,139 @@ | |||
*{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again avoid * selector
popup.css
Outdated
display: inline-block; | ||
} | ||
input{ | ||
-webkit-transition: 0.3s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need any browser-specific prefixes, since our extension is only for Chrome.
popup.css
Outdated
} | ||
input{ | ||
-webkit-transition: 0.3s; | ||
transition: 0.3s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing? It seems like it's missing a property based on the way it's described here.
popup.css
Outdated
} | ||
|
||
input[type=text]{ | ||
width:50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using ems, since it's for text.
popup.css
Outdated
border-radius: 5px; | ||
outline: none; | ||
text-align: center; | ||
font-size: 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use default unit; explicit is almost always better than implicit.
popup.css
Outdated
width:50px; | ||
padding: 12px 12px; | ||
margin:auto; | ||
box-sizing: border-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need this with the * (which should be body) selector.
popup.css
Outdated
background-color: white; | ||
} | ||
#hotkey{ | ||
width: 30px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ems?
popup.css
Outdated
width: 30px; | ||
} | ||
.table #url{ | ||
width: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ems
popup.css
Outdated
.table #url{ | ||
width: 50px; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete blank lines for my OCD
popup.html
Outdated
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<title>KeepTabs Pop-Up</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Popup is a technical term in the domain of Chrome extensions. Give it a title meaningful (and that doesn't sound bad -- popup sounds like an ad) to the user, e.g. KeepTabs Options.
overlay.html
Outdated
</head> | ||
<body> | ||
<div id="top"> | ||
<img src="https://openclipart.org/download/215361/shadowed-news-icon.svg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From "openclipart" it sounds like this is fine, but make sure you've confirmed this is okay to use without any attribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a filler image for now! currently trying to design a new keeptabs logo
popup1.html
Outdated
@@ -0,0 +1,48 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually some profound truth in the old quip that there are two hard problems in CS: cache invalidation and naming things (I'm talking about the naming). It forces you to think about and understand what something is for -- whether that's a variable, function, or file. It's a very useful exercise. Try to get into the habit of giving everything a meaningful name. It's very important when you're working with others (which all real software projects will be), and you look silly in an interview if you name things "thing", "thing1", etc.
popup1.html
Outdated
</div> | ||
|
||
<div id="domain"> | ||
<input type="button" value="Add new KeepTab"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this turning "KeepTab" into a noun. It can easily get confusing when you use the app's name for specific concepts within the app. We should talk about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start! Impressed with what you've got done so far. Let me know if you have any questions about my comments and sorry in advance for being very picky and brusque. I'm very tired and you get used to typing as little as possible with code reviews after a while :P
Nawon's time to get roasted by Jemmin. |
No description provided.