Progress Bar Component - Githubissues

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 Bar Component #119

Closed nastiatikk closed 9 years ago

nastiatikk commented 9 years ago

Visual 'Progress is' is a part of u-hidden utility that is not included to Stencil. It's an accessibility solution (cause aria-label doesn't work for some reason) to define what is this % about. p

Please review this component and leave your notes. Meanwhile I am gonna work on the dust file for this component.

Todo

[x] Remove c--radius and c--round variations
[x] Remove the visual label (keep the accessible label)
[x] Consider minimizing the configurable variables
[x] Add a Table of Contents link to Progress Bar Component in root Readme file
[x] Designer +1 @ry5n
~~Designer +1~~ (close enough!)

nastiatikk commented 9 years ago

@jeffkamo @avelinet @cole-sanderson @ry5n @kpeatt @mlworks

avelinet commented 9 years ago

@nastiatikk I made some minor changes to the visual tests wording, mostly just grammar stuff :)

nastiatikk commented 9 years ago

Thank you @avelinet!

ry5n commented 9 years ago

This is good work. There are some things I’d suggest changing, my first thought is that I haven’t seen us implement this component on a build before. Have we done it? Should it be a priority?

nastiatikk commented 9 years ago

We had some kind of progress-bar on Carnival project but because of its design we needed to use desktop logic and approach (in addition to that they decided to remove it later). Otherwise I haven't seen it in any of our projects. So I'd say that priority for this component is minor.

Though I'll be happy to hear your ideas and implement them.

ry5n commented 9 years ago

Thanks for the input. Let’s hold off on this for the moment. I’ll loop back sometime within a week.

ry5n commented 9 years ago

So here are my suggestions:

Let’s remove the c--radius and c--round variations. Let’s keep things simpler (devs can customize appearance in their projects)
I don’t think the label will be flexible enough to style (it has no element wrapper). Let’s leave the visual label out for the simplest version
I think we have too many configurable variables. We definitely don’t need font-size, spacing or the radius/round variables. I think height, color and background color are all we really want and we could even get away with none.

nastiatikk commented 9 years ago

Removed c--radius and c--round.
I would keep the label. It's a valuable piece of a progress-bar and it's not a big deal to wrap it with a span tag. Otherwise if label required how it's gonna be added to the component if there is no element in the markup for that?
Removed unused variables

nastiatikk commented 9 years ago

I've made all requested changes. Still think that label should be kept as a valuable piece of a progress bar.

Can be reviewed again.

jeffkamo commented 9 years ago

Might want to pull from master. Looks like there's a conflict just waiting to be resolved.

jeffkamo commented 9 years ago

@nastiatikk is that last to-do still need to be done?

nastiatikk commented 9 years ago

No @jeffkamo It's already removed and kept for accessibility reason only

ry5n commented 9 years ago

I think this is good to go in. We should take another look when we migrate to Stencil 2.0.