Add placeholder classes to remaing utils - 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

Add placeholder classes to remaing utils #104

Closed jeffkamo closed 9 years ago

jeffkamo commented 9 years ago

Status: Opened for Review Reviewers: @kpeatt @ry5n @avelinet @cole-sanderson @mlworks @nastiatikk JIRA: CSOPS-954

Changes

Add placeholder classes (i.e. $u-width-1of3) to all utilities

ry5n commented 9 years ago

~~Otherwise +1~~ -1 I’m just not sure right now.

jeffkamo commented 9 years ago

Haha @ry5n.

Do we want to totally cancel this then? If so I can close this and make a PR to remove the placeholder class that was previously merged on visibility.

ry5n commented 9 years ago

Sorry, no, don’t cancel this yet. The visually-hidden placeholder is genuinely useful. Maybe a few others too, like all-caps. But @extend %float-start which sets float: left seems of questionable value (except for RTL sites maybe). Widths definitely: @extend %u-width-1of2 instead of width: 50% seems like kindof a facepalm. Certainly if we go forward I’d rather just write out flex: initial I think with a top-level comment.

kpeatt commented 9 years ago

@ry5n This is blocking us on releasing 1.0 right now. Have any additional thoughts on this?

ry5n commented 9 years ago

For these utils in particular, I don’t think there’s an actual use case for placeholders. However, the argument would be for consistency: if we do some utils as placeholders then we should do them all. The implementation here bugs me only aesthetically (I would rather just see flex: initial added to each declaration) but I won’t block merging this, and I support it if you are both +1.

kpeatt commented 9 years ago

@ry5n Let's add flex initial to each declaration. It fixes a concern @jeffkamo and I had with discoverability too.

ry5n commented 9 years ago

OK

kpeatt commented 9 years ago

@ry5n Okay I think we're GtM on this.

ry5n commented 9 years ago

+1. (And sorry for being so cranky)

kpeatt commented 9 years ago

Were you cranky?

nastiatikk commented 9 years ago

However, the argument would be for consistency: if we do some utils as placeholders then we should do them all.

@ry5n @jeffkamo I don't think that in this case we benefit from consistency. If we're not supposed to use some %extends maybe it's worth not to have it on the elements that don't need it?

jeffkamo commented 9 years ago

@nastiatikk are you proposing that some be removed? Maybe it's better to create an issue to discuss this in rather than in this old PR.