search

mobify / stencil

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

Utility display layout #137

Closed nastiatikk closed 9 years ago

nastiatikk commented 9 years ago

Status: Ready for Review Reviewers: @avelinet @ry5n @jeffkamo @yourpalsonja @cole-sanderson @kpeatt @mlworks

Changes

Display utilities added The same as with colour utilities - sometimes we need just one line of code to make the page look fine.

Todos:

jeffkamo commented 9 years ago

I'm not 100% sure if these properties should be accessible as a set of utility classes, but if so then :+1:

nastiatikk commented 9 years ago

I met several times the issue when I needed display: block be applied on <span> with no chance to change it to <div>. And last time create a utility for this.

ry5n commented 9 years ago

I’d probably shy away from these in responsive builds, but for more device-specific builds they should be OK. The question is if we do more responsive builds (which I am a fan of) then we may need to remove these at some point. Hence my hesitation.

nastiatikk commented 9 years ago

If the project gonna be responsive after it was deployed as a device-specific build we will just refactor it and remove this class if it breaks something.

ry5n commented 9 years ago

I’m speaking more to the possibility of deprecating these and removing them from Stencil at some point if they no longer reflect best practices. We can’t add every useful thing – we should try to strike a balance between usefulness and the overhead required both maintenance and learning.

nastiatikk commented 9 years ago

Thank you for clarifying, I see you point now. But shouldn't we think about this when we build at least more then two-five responsive projects per year? When it becomes an issue?

ry5n commented 9 years ago

Just found out we’re already doing a new responsive build: an existing mobile site being augmented for tablet. I think we should be careful to make sure all our smartphone builds going forward are friendly to upgrade in this way. So I’m still not sure about these utils being part of the core library.

avelinet commented 9 years ago

I'm also not sure about this utility, can't say I've yet encountered a need.

nastiatikk commented 9 years ago

Seems like it looks redundant for majority. I'm closing this PR. If we need this util we can always create it in the project.

ry5n commented 9 years ago

OK. Let’s keep an eye on the need for this and similar utilities. We can always revisit at a later date.