themovation / th-widget-pack

Theme Widget Pack
19 stars 6 forks source link

Service Blocks - Markup ready #47

Closed teaganm closed 8 years ago

teaganm commented 8 years ago

The template file displays the different service block options.

For the alignment options, the first element 'th-service-blocks' gets a different class for left, right, center. The next element, 'th-sb-single' gets a different class depending whether the icon is standard, circle, or border style. The next element, 'th-sb-icon' gets a different class depending on the alignment and icon options.

In the widget form, I’d like to slightly change the ‘Align Service Blocks’ part. I’d like these options:

Let me know if there is any issue with any of that. Thanks Adi!

adiraoco commented 8 years ago

@teaganm I am working on this in the branch https://github.com/ryanlabelle/themovation-so-widgets-bundle/tree/serviceWidget

I have placed my code above yours.

Couple of points : Point one : The markup you have added for the icon and link is -

<div class="th-sb-icon med-icon">
    <a href="http://www.google.com">
        <span class="sow-icon-glyphicons" data-sow-icon="&#xe364;" ></span>
    </a>
</div>

Could we change it to -

<a href="http://www.google.com">
    <div class="th-sb-icon med-icon">
        <span class="sow-icon-glyphicons" data-sow-icon="&#xe364;" ></span>
    </div>
</a>

This way the icon markup is inside the link markup.

Point two : For the icon style standard the mark you have is -

<div class="th-sb-icon med-icon">
    <span class="sow-icon-glyphicons" data-sow-icon="&#xe364;" ></span>
</div>

Everywhere else the standard icon just outputs the icon like so - <span class="sow-icon-glyphicons" data-sow-icon="&#xe364;" ></span>

Could we stick to this? Or would you like to keep it the same.

Any thoughts? Thanks.

teaganm commented 8 years ago

Hi Adi:

Point one - yes that's fine.

Point two - just to confirm you are only suggesting a change for the standard icon type right? The circle and border types would remain the same? Then that is fine.

adiraoco commented 8 years ago

Implemented in bb0c27b184361637d84f50cb662b01e5ff37b8d0. Let me know if there is any problem. :)

teaganm commented 8 years ago

Thanks. Just one thing I noticed in testing is that when the 'Center - Vertical' option is selected along with the Circle or Border option, the the classes are supposed to be:

'circle-lrg-icon' 'border-lrg-icon'

Basically just 'lrg' instead of 'med' like the left/right options.

If that's not a quick change, then I can work around it.

adiraoco commented 8 years ago

@teaganm This would make the function I have added for displaying icons a little messy.

That being said; I have added a provision to add additional classes to .circle-med-icon and .border-med-icon. So how about adding a class for Center - Vertical like .cv-lrg. This would make these classes .cv-lrg.circle-med-icon and .cv-lrg.border-med-icon

What do you think?

teaganm commented 8 years ago

Ok sounds good, that will work just fine.

adiraoco commented 8 years ago

@teaganm I have added the class. Please check. Thanks. ceb1fa63c000ae4f82f14d874914693bf125f2b2

teaganm commented 8 years ago

Looks great Adi, thanks.