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 List component #133

Closed nastiatikk closed 9 years ago

nastiatikk commented 9 years ago

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

Changes

ry5n commented 9 years ago

Can you describe what problem this component is solving? It seems to me that we have the %ul and %ol placeholder classes for restoring the default look for lists. The dust template we have here is more difficult and also more limited than just writing out the markup manually.

nastiatikk commented 9 years ago

By default we have list-style-type and margins/padding being nulled. We also have %ul to extend in Vellum. But we don't always have page styles to extend to. In this case we could create a list component in the project and extend Vellum there, or use this component.

It looks complicated only because of the opportunity of nested list. But if there is no nested lists it also works fine.

nastiatikk commented 9 years ago

But honestly speaking I didn't know we have %ul to extend and usually created c-list component in the project, because by default we don't have list styles applied to the lists

ry5n commented 9 years ago

OK, yeah. I think there’s an argument to be made for a c-list component (or a c-ul and c-ol). It’s actually a good one to think about because it’s probably the most trivial component we could make. I’m not sure it even makes sense to have a dust file, or a schema, or anything other than the stylesheet.

@jeffkamo what’s your take?

nastiatikk commented 9 years ago

We can ask front-end developers if they find dust for list valuable or no

jeffkamo commented 9 years ago

I'd be fine with having a component like this available.

Altough if it's just a matter of making more default bullet lists and ordered lists available, there are other options we can consider too, such as opt-in typography, in which case we'd need no classes and therefore no dust templates (as html would be enough).

Thoughts on that?

nastiatikk commented 9 years ago

I like this idea, and I've been thinking about the approach of not having default styles applied to the tags but instead have default classes to style default elements. We could definitely think about it.

nastiatikk commented 9 years ago

But if we do this it will be a big change in Vellum. Probably we can continue with c-list component and then (maybe by that time Stencil 2.0 is live) and we simply won't move it further?

avelinet commented 9 years ago

Thanks for starting this @nastiatikk. A list styled component would definitely be useful for those places where we do want list styles, while keeping the global list styles neutral. I actually ran into that on my last project. I think c-ul and c-ol would be good.

I'm not convinced we need a dust template for this however.

And while I'm intrigued by the opt-in typography approach, I think a list component will be more flexible for our use case. For example when we are mostly pulling in a large block of desktop markup and just want to apply some list styles interspersed in it.

nastiatikk commented 9 years ago

If we have dust file it won't hurt. I've asked engineers and though just 2 of them answered (@bendvc and @ericmuyser) but they told that they'd prefer to have dust file. But I'd collect more feedback on this questions.

Speaking about classnames, c-ul and c-ol sound more then valid. But in this case we'll need to have 2 components with the same content, or 1 component with a filename not matching component selector.

We could have c-list c--ul and c-list c--ol. Or just c-list with bullet styles (as it's the most popular list) and modifiers for ordered list, for example c--ordered.

But if we decide to keep dust with ol we will need another template file (or complicate it with if statements)

jeffkamo commented 9 years ago

...I think a list component will be more flexible for our use case. For example when we are mostly pulling in a large block of desktop markup and just want to apply some list styles interspersed in it.

I don't think I agree. What flexibility does that give you? It just sounds like more heavy lifting to me, because in this case you have to...

  1. Select all the lists
  2. Identify and capture the list data
  3. Insert that data into the list template
  4. Inject the compiled template back into the content

...as opposed to just inserting a single class on the content container.

In my mind, a dust template is most useful when we know lists are going to be present for specifically formatted content. The example I'm thinking of is something like a feature list on a PDP page.

avelinet commented 9 years ago

@jeffkamo I was talking about a list component without the dust template.

jeffkamo commented 9 years ago

@avelinet ah, I see, that's fair.

Okay where are we with this component? Sounds like overall we like the idea? For @avelinet it sounds like your use case is still valid whether there is a dust file available or not, so I don't see much issue with this being created. Thoughts?

ry5n commented 9 years ago

I’m still iffy on the dust template to begin with. The best argument I heard when we canvassed other folks was the ability to easily style several list types, including something like a visual checklist (checkmark icons as bullets). One could implement that with just CSS and generated content, except if you’re using inline SVG.

Overall I don’t want to move on this right now.

nastiatikk commented 9 years ago

I wouldn't mind to remove dust template if nobody finds it useful. Imagine there is no dust (I'll clear later today) and continue your thoughts =)

ry5n commented 9 years ago

Don’t take it away yet. I think we need more time to come to some consensus on this.

jeffkamo commented 9 years ago

Is there a down side to having the dust templates? I see them as still being useful: They are available if you want to compose with them, they can be used to add components to Almanac, or you can ignore them and just use the CSS classes in your own markup. Not using them adds no weight to a project.

nastiatikk commented 9 years ago

This was my point originally - that if we have dust template it won't hurt anything and anybody. But if dust template is a blocker of c-list component, I'd rather prefer not to have it then wait for another 6 months until we find a solution on this question.

ry5n commented 9 years ago

The downside to having a dust template that we don’t really need is that we need to maintain it, and people might think they need to use it. If we don’t need it, we shouldn’t have it.

jeffkamo commented 9 years ago

hmm, maybe that's true. But for me, if a dust template is missing, I'm just going to make it anyway. Make it for Almanac. So not having a dust file isn't going to save me any time.

But maybe that's just me? Or maybe at that point the dust template would just be a bunch of static html that acts as example markup.

Regardless, something's going to be made (either example html or a dust template with mock data) one way or another. Having a dust file (and some mock data) pre-built removes that time sink.

ry5n commented 9 years ago

Hmmm… the Almanac use case does make some sense. But then are we going to make templates for every html structure? tables, headings, paragraph.dust? What is our solution for those?

jeffkamo commented 9 years ago

Good question. I guess that depends if those become stencil components. At the moment, those are only styled in Vellum. I doubt the practicallity in being so granular with components. We don't need to rebuild all html elements as Stencil components.

ry5n commented 9 years ago

We often create a .c-heading, and I doubt that needs a dust template. I feel the same about lists. However, I am feeling increasingly weird about the notion of a component that is only css. Could this kind of thing be a util as well? I feel a bit nuts contemplating it, but mayyyyyyybe??

kpeatt commented 9 years ago

If we want our definition of utility to be something that is CSS only then that works for me.

ry5n commented 9 years ago

Cool. Let’s contemplate that.

jeffkamo commented 9 years ago

philosoraptor-t2

nastiatikk commented 9 years ago

Hm... all our components are css only. I wouldn't use this as a utility definition. And I know that some engineers would build list as a dust component, depending on the project though. And in this case it's not utility anymore, though have u-prefix?

kpeatt commented 9 years ago

@nastiatikk that's not true, all of our components have dust partials and schemas attached to them.

nastiatikk commented 9 years ago

So in this case button, alert, badge, icon, probably spinner - are also utilities? They have dust templates in Stencil, but those templates are never used in the projects.

jeffkamo commented 9 years ago

I could forsee those using templates quite easily.

{>"component/icon" name="hamburger" /}
{>"component/alert" text="alert message" /}
{>"components/badge" value="30" /}
{>"components/button" type="button" text="I'm a button" /}
{>"components/spinner"/}

...or better yet, using Ryan's component syntax:

{@c-icon name="hamburger" /}

{@c-alert}
    alert message
{/c-alert}

{@c-badge value="30" /}

{@c-button type="button"}
    I'm a button
{/c-button}

{@c-spinner /}
nastiatikk commented 9 years ago

How about c--secondary button and other modifiers? Alerts that mostly are ajaxed?

jeffkamo commented 9 years ago

Hmm, modifiers could probably be done a few different ways. I think this is probably the most flexible?

{@c-button type="button" class="c--secondary"}
    I'm a button
{/c-button}

For Ajax, you just have to compile the template. Probably something like...

var buttonData = {
    type: 'button',
    class: 'c--secondary',
    content: "I'm a button"
};

buttonTemplate.compile(buttonData, function(button) {
    // insert the compiled `button` into the DOM
});
ry5n commented 9 years ago

OK so the reason I dislike the dust template for lists is because the template would end up being just a normal html list but with a class on the <ul>. That’s the reason I don’t think the template is warranted.

For many other components, including icons, alerts, spinners and maybe buttons, the html is more complex, so it warrants a template.

For ajax changes, content can be re-rendered in view scripts using dust templates the same as it is now.

For modifier classes, all component templates should accept a class key so you can do {@c-button class="c--secondary"}Text{/c-button}.

ry5n commented 9 years ago

@jeffkamo jinx ;)

jeffkamo commented 9 years ago

:D

nastiatikk commented 9 years ago

There is nothing complex about those htmls

<div class="c-badge">
    {quantity}
</div>

{#alert}
    <div class="c-alert {class}">
        {message}
    </div>
{/alert}

<a class="c-button" href="#">Anchor Button</a>
<input class="c-button" type="submit" value="Submit Button" />
<button class="c-button">Button Button</button>

<p><span data-icon-name="checkmark"></span> checkmark</p>

<div class="c-spinner">Loading...</div>

List template looks much more complicated then these dust files

nastiatikk commented 9 years ago

I am not vouching for list template anymore, I am more curious why we have templates for these ^_^ components if they're nothing but more simple html?

jeffkamo commented 9 years ago

I suppose at this point it's syntactic sugar?

It removes the need to remember how a component could be structured and softly enforces consistency in how those components are rendered.

The parts that are redundant (the class and element type) can be ignored. You would only have to remember the name of the components, and any values that component needs to receive.

nastiatikk commented 9 years ago

But isn't it something you can see on the test page?

jeffkamo commented 9 years ago

Perhaps this really only applies to more complex components, but the point would be to reduce the need to reference anything to know what you need to include a component on a page.

To add an icon, all you need to remember is the name. No need to decide if it's a span, an i or whatever – that's already decided. Just {@c-icon name="hamburger" /}

ry5n commented 9 years ago

So for icons, I’m thinking more of the svg variety, where you write something like this:

<svg class="c-icon">
   <title>Accessible title (optional)</title>
    <use xlink:href="#icon-checkmark"></use>
</svg>

So that is much easier to express as {@c-icon name="checkmark" title="Checkmark" /}.

Alerts probably require more complex html, with an optional dismiss button for example. The spinner may or may not be implemented in pure CSS – and we could change the implementation without affecting the usage. For buttons, you probably don’t need a template either, but there is a reason I listed them here as a maybe:

That’s the main reason for the dust templates: they’re an abstraction that hides away complexity behind a easy-to-use API. Beyond that, the vision is that if you use a component in this way, we’d have automatic attachment of behaviour so you don’t have to separately init() the ui script (behaviour).

jeffkamo commented 9 years ago

So @nastiatikk to answer you question "why have templates for simple components?", I suppose the answer(s) would be:

That second point is, to me, the main reason why I would want templates even for simple components.

nastiatikk commented 9 years ago

Sorry @jeffkamo I'm totally lost in understanding your words in accordance with @ry5n words.

OK so the reason I dislike the dust template for lists is because the template would end up being just a normal html list but with a class on the <ul>. That’s the reason I don’t think the template is warranted

Because all mentioned above templates are simple templates with normal html only.

jeffkamo commented 9 years ago

My first point...

  • We don't need templates, as markup itself may be simple enough to not need a template

...aligns with Ryan's quote...

OK so the reason I dislike the dust template for lists is because the template would end up being just a normal html list but with a class on the <ul>...

In my remaining points: I am pointing out that despite how simple the markup may be, there are other benefits to having a dust template (i.e. Almanac).

So I'm agreeing. Yes, the markup is simple. Yes, that means a template isn't actually necessary. However, by not having a dust template we are loosing out on some other things (i.e. easy Almanac integration).

nastiatikk commented 9 years ago

Thanks, got it now! Was confusing because your points were all in one group and first one was the only one repeating @ry5n position. As the others were confronting it =)

jeffkamo commented 9 years ago

Sorry about that, I should've been more clear. My bad!

jeffkamo commented 9 years ago

Was the debate about whether to include a dust template the only thing that was blocking this PR?

jeffkamo commented 9 years ago

So I believe in Stencil 2.0, all stencils will have a dust template regardless of simplicity due to the minimum feature requirements that 2.0 stencils will have (i.e. customizable class and optional id and role properties). With that in mind, shall we move forward with this PR with the dust template left in place?

ry5n commented 9 years ago

@jeffkamo I’m not entirely sure that every component in Stencil 2.0 should have a dust template. It’s undecided and one of the things I’m a little worried about. Consistency is important, but no sense having a template if it’s not needed (performance overhead and actually reduced flexibility).

If anything, I think this may be one of those things that should be a utility class in Stencil 2.0, not a component. Unfortunately that is also still somewhat of a murky area; I’ve done some exploratory work on it (I started implementing Arrange as a util) but I have concerns with making something like Arrange (and possibly this List component) into a util.

jeffkamo commented 9 years ago

@ry5n fair enough. I guess we can leave any remaining dust template questions for Stencil 2.0, but in the mean time is there any reason why we should not allow the dust file in this PR to remain? I'm leaning towards "leave it in because we'll refactor as necessary in 2.0" whatever that may look like. I'd rather do that than go back and forth and never close this PR.