Closed jrubenoff closed 9 years ago
:clap:
Great job! This is truly a masterpiece... I really like how you've refined the visual styles. All of my feedback is minor:
<select>
isn't filling 100% height (http://take.ms/Vb2Xp and http://take.ms/9OEKy)The copy here is a bit confusing. In this particular example, does the minus icon indicate that more than one response has been selected and Clay is assigned to some of the them but not others?
Some other thoughts...
Thanks, Kari and Adam! Addressed most of your comments. Some of these problems occurred because I didn't specify Source Sans Pro correctly, so I'd recommend you take another look at headers, etc.
There's a perceptible delay when checking radios and checkboxes
Can't reproduce this in Safari, FF, or Chrome. What browser are you using?
Rename button size http://take.ms/3aCzV
It's supposed to be that large, to match the h2
.
Filter dropdown thick border
We can change to this if it's really an issue, but IMO it's uglier:
As for the splash footer, I wanted to take that out of dvl-core
, but let me redesign it instead.
Let's create two variants of the hero image. Not sure how to change the layout depending on each page... can you handle that?
Feel free to refactor the partials... as long as the design doesn't change, I don't care.
Also, I barely touched flashes. Didn't want to step on the work you've done. We can merge that in after this.
As for the splash footer, I wanted to take that out of dvl-core, but let me redesign it instead.
We can take it out, but if we do, we should put it in a repository somewhere where all of the apps using it can vendor it in. (Splash, knowledge base.... maybe that's it.)
Can't reproduce this in Safari, FF, or Chrome. What browser are you using?
Chrome. This "issue" exists in master
too, but I think I'm noticing this here because the element is bigger. The transition on the checkbox and radio icons just makes them feel sluggish. Maybe we can speed them up a little bit? I don't know, there has to be a reason why they feel more sluggish in this branch....
It's supposed to be that large, to match the h2.
Right, I think it's too prominent for a button that will maybe be clicked once. Can we just remove the border maybe?
We can change to this if it's really an issue, but IMO it's uglier:
Unfortunately I think it is an issue... especially when combined with the "multiple sections" (visible in the assignment dropdown w/ teams) it looks really off.
Let's create two variants of the hero image. Not sure how to change the layout depending on each page... can you handle that?
Yup, of course.
I'm not so sure about orange as a primary button color. Am I the only one that feels this way?
You checked this off, but I don't see any code changes, or any response to this in your comments.
I finally grokked the thing about .subtle
buttons being hidden instead of disabled... I still don't totally get why that behavior exists, or when we'd use it. But maybe that's just me.
Actually, the radio / checkbox behavior appears to be a lot better in Screendoor than in the style guide... maybe we should investigate during our check-in today.
This looks great! Looking forward to having more re-usable components and design patterns/guidance.
I'm up for changing the filter dropdown, but since I don't have a problem with it, I wanted to make sure we're talking about the latest commit:
Is that still too weird? If so, no worries. I'll change it.
Just fixed everything else you mentioned.
No; that solves my only real problem with it. I think I can get used to the thick border.
It looks like pretty much all of the comments have been addressed, besides the ones about the styleguide itself. I can work on those today.
As you mentioned yesterday, I think we should add some documentation around icons. I'm not really sure how we're using font-awesome and our own iconset at the same time.
Currently we're not using them at all, I just committed the fonts and styles to the repo as a starting point.
How about we delete the icons, merge this, and then add them back in a subsequent PR?
That would certainly make sense :D
Just made some commits where I said I would. Two more unimportant comments:
Are cssgradients
the only thing we're using modernizr for? If so, I'd like to replace our build with this one: http://modernizr.com/download/#-cssgradients-cssclasses
Sure, I just need those CSS classes.
On Thu, Jul 2, 2015 at 11:28 AM, Adam Becker notifications@github.com wrote:
Are cssgradients the only thing we're using modernizr for? If so, I'd like to replace our build with this one: http://modernizr.com/download/#-cssgradients-cssclasses
— Reply to this email directly or view it on GitHub https://github.com/dobtco/dvl-core/pull/105#issuecomment-118117027.
I think we can merge this in whenever Screendoor's PR is ready.
I think the content of this PR is ready, but I'd like a chance to quickly refactor a couple of things before merging:
Can you also address this one item from above?
Another question: Adding .modal_footer_actions
touches a lot of code, and I'm not entirely sure why we need it: https://github.com/dobtco/dvl-core/blob/redesign/vendor/assets/stylesheets/dvl/components/modals.scss#L145-L166
The comment on line 145 says "Footer (for actions)". And what's the different between footer_actions and footer_link?
Here are some bugs in IE9 (sorry):
Is there a way that we're vertically aligning things that doesn't work in these older browsers? I'd like to know, too...
Just caught a code issue:
We appear to be distinguishing between our two pagination components using + .pagination_status
like so:
This strikes me as not very maintainable and not very performant. Why not just create two separate components?
I just added vertical alignment mixins for margin-top
and top
, to fix IE9 issues.
I was waiting to refactor .pagination_status
in a future PR to minimize code changes in Screendoor, but since we apparently only use it once, I just refactored it. Also renamed the modal footer containers to .modal_footer_primary
and .modal_footer_secondary
... they're for primary and secondary actions.
You can add -ms-interpolation-mode: bicubic
for images, which I just did, but it still looks pretty bad. To improve it further, I think we would need to use IE filters, which require extra inline styles, so I'd rather not.
That's everything.
It works in IE9! Consider me extremely impressed :)
Here's one bug though:
Just fixed that.
I know Screendoor's PR has a couple of outstanding issues, but it seems like this one is good to merge.
I'm not sure why there is a margin being applied, so I'll wait to make any changes.
Merged in #114.
Alright, here's a start.
Right now, the
dvl-core
app mostly serves as a visual unit test and outdated pattern library. This PR:Our apps will need some adjustment before merging this in, but this PR is intended to have as little external impact as possible. I'll be renaming components, changing others, and generally saving other "breaking changes" for subsequent releases.
This is mostly about how things look, so anyone on @dobtco/team is welcome to give feedback. Just follow these steps:
script/bootstrap
, thenscript/server