mobify / stencil

DEPRECIATED - The latest Stencil development is currently taking place in the Adaptive.js repo.
MIT License
4 stars 0 forks source link

Progress Component #108

Closed avelinet closed 9 years ago

avelinet commented 9 years ago

This PR adds the Progress component

Status: Ready for Review Reviewers: @kpeatt @ry5n @jeffkamo @cole-sanderson @mlworks @nastiatikk JIRA: CSOPS-515

Minor work todo

nastiatikk commented 9 years ago

@avelinet What do you think about tweaking a little background color for complete and current steps? They almost don't differ from each other and you can't see current step right away.

Thought it's just a performance note, but I believe that performance should show you very clear what is where.

We could tweak background like this:

$progress__current-bg-color: darken($progress__item-bg-color, 10%) !default;
$progress__completed-bg-color: darken($progress__item-bg-color, 20%) !default; 

And also I'd some spacing on the right for the last step.

I don't know if we should bother about default design much, but with the arrows approach last step looks like it was chopped. It will look good if the width is 100% of the viewport but if it has space on the sides it will require additional styling.

And the very important thing (I think) - current step should be visible, not cut.

Besides that it looks like a usable component!

ry5n commented 9 years ago

Solid work. Basically it’s some nitpicks about naming choices, whether or not we move the root class back to the root node, and otherwise +1.

Just to clarify, I think the only thing that’s a blocker for merge is the component name itself. I do think that should be clarified. Everything else is minor.

avelinet commented 9 years ago

@ry5n I've incorporated your feedback, thanks!

jeffkamo commented 9 years ago

Wait, why are there files in dist/templates/? Are those outdated files? Pretty sure there isn't a template directory in dist, or am I mistaken?

avelinet commented 9 years ago

@jeffkamo The template files in dist are out of date. After all this thing dates back to December! I've removed them.

jeffkamo commented 9 years ago

@avelinet I think you removed the wrong template files. The root /template directory is fine as is, it's the /dist/templates files that I think need to be removed

ry5n commented 9 years ago

Not sure about what’s happening with the templates directory, but once that’s resolved we can merge. I still think the top-level node should have the root class, and the dust template issue I mentioned above, but we could make those new issues for later. +1 on merge.

avelinet commented 9 years ago

@jeffkamo @nastiatikk @ry5n, appreciate all the feedback on this. I reverted the removal of the wrong template files and removed the correct ones. Asides from issues with comment placement and semantics, discussions on root classes and dust templating wrapping keys, all of which require further discussion until the 🐄s come home... Are there any blockers remaining or can this be merged in?

ry5n commented 9 years ago

Nope this is good to merge, the rest are bikesheds! Next one to +1 gets to merge!

jeffkamo commented 9 years ago

Aah, not quite. Aveline and I realized that the comments are actually rather confusing. We're going to clarify those after lunch, and at that point we'll most likely be good to merge!

jeffkamo commented 9 years ago

+1'ed!