-
Notifications
You must be signed in to change notification settings - Fork 34
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
Bit of styling and rewording #86
base: something-tracker
Are you sure you want to change the base?
Conversation
rebased on |
hmmm no idea why tests fail, I'll check it later, we can merge this as is, here’s a preview (without sign up / sign in working): https://rawgit.com/hoodiehq/my-first-hoodie/espy-styles/www/index.html |
Ah, I just noticed I forgot the username/password fields because I was logged in all the time. I'll do those soon. |
textarea:focus{ | ||
border-color: transparent; | ||
outline: none; | ||
box-shadow: 0 0 0 3px #222; |
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 looks like it could be worse for a11y, I'd rather have 'ugly' focus states than not so obvious ones. Would make for less code too.
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.
Agree :)
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 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.
Okay I'm finding your one better, sorry, I was going off of Nicks general thoughts. I'd be happy to have this one :)
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 think the default outlines are platform/browser specific some browsers do it really well, this looks great @espy, better to have something consistent then hoping the browser does the right thing..
Are you sure you want to put them on hover too?
Looking good! Does this make sure that the inputs at the top of the screen are using consistent styles with the rest, noticed the button at the top in the screenshot is a lot smaller than the others used. Theres some other things odd with the inputs at the top so maybe we could address them separately and not block this. |
@NickColley I forgot the ones at the top because I was logged in all the time, and they're not visible then. I'll fix that soonish, along with a few other details concerning responsiveness. |
input, | ||
textarea, | ||
table{ | ||
border: 1px solid #aaa; |
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.
Think this could go with https://github.com/hoodiehq/my-first-hoodie/blob/espy-styles/www/assets/app.css#L97 and avoid styling the core elements.
Just making a PR for your PR, PR. |
chore(refactor): Make changes based on review
I’ve created https://github.com/hoodiehq/hoodie-app-something-tracker now, can you start the PR again for that repo? I didn’t touch assets much, only added password reset button |
Nicer buttons with hover and focus states, tidier text alignments, and
normalize.css
is now part of the repo, because I found myself working on this on the train, offline, unable to load it from github.I also changed all instances of
login
,log out
andregister
tosign in
,sign out
andsign up
to match the Hoodie API.Also,
submit
is nowadd item
, so that corresponds with the use ofitem
in other places.Now looks like this: