Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Twitter Bootstrap Html Helper Class. #1294

Closed
wants to merge 19 commits into from

Conversation

@kartik-v
Copy link
Contributor

@kartik-v kartik-v commented Nov 22, 2013

Creating a Twitter bootstrap specific HTML helper class. This is an extension to PR #1285 which was closed.

Creating a bootstrap HTML helper class. This is an extension to PR #1285 which was closed.
@coveralls
Copy link

@coveralls coveralls commented Nov 22, 2013

Coverage Status

Coverage decreased (-0.03%) when pulling 9014b11 on kartik-v:patch-4 into b20e576 on yiisoft:master.

@coveralls
Copy link

@coveralls coveralls commented Nov 22, 2013

Coverage Status

Coverage decreased (-0.38%) when pulling 32eb227 on kartik-v:patch-4 into b20e576 on yiisoft:master.

kartik-v added 7 commits Nov 22, 2013
Included more helper functions.
Minor clean up of code.
More code cleanup.
…pes, and components.

Created all common functions that need not be widgetized. Complex helper functions like ```mediaList```, ```address```, ```blockquote``` are also included. Added documentation and examples for each function. List of bootstrap helper functions available:
1. icon
2. label
3. badge
4. jumbotron
5. panel
6. pageHeader
7. well
8. media
9. mediaList
10. closeButton
11. caret
12. lead
13. abbr
14. blockquote
15. address
16. staticInput
Created helper functions for bootstrap functionalities - that would not need a widget. Complex helper functions like ```listGroup```, ```mediaList```, ```address```, ```blockquote``` are also included. Added documentation and examples for each function. List of bootstrap helper functions available:
1. icon
2. label
3. badge
4. listGroup
5. jumbotron
6. panel
7. pageHeader
8. well
9. media
10. mediaList
11. closeButton
12. caret
13. lead
14. abbr
15. blockquote
16. address
17. staticInput
Updated static input comments
Since ```BaseHtml``` already has a label function.
@sv3tli0
Copy link
Contributor

@sv3tli0 sv3tli0 commented Nov 24, 2013

I want to suggest about Panel method adding 4 types of content : 'start', 'end', 'beforeBody', 'afterBody'.
And other thing is instead inputing all items 1 per 1 send them as 1 array $items.

public static function panel( $items = [], $options = [], $type = 'default' )
{
     static::addCssClass($options, 'panel panel-' . $type);
     $html = ( !empty($items['start']) ? $items['start'] : '' );
     $html .= ( !empty($items['heading']) ? static::tag('div', $items['heading'], ['class'=>'panel-heading']) : '' );
     $html .= ( !empty($items['beforeBody']) ? $items['beforeBody'] : '' );
     $html .= ( !empty($items['body']) ? static::tag('div', $items['body'], ['class'=>'panel-body']) : '' );
     $html .= ( !empty($items['afterBody']) ? $items['afterBody'] : '' );
     $html .= ( !empty($items['footer']) ? static::tag('div', $items['footer'], ['class'=>'panel-footer']) : '' );
     $html .= ( !empty($items['end']) ? $items['end'] : '' );
     return static::tag('div', $html, $options);
}
Updated bootstrap panel function to include configurable content (passed as an array). Also included usage examples.
@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage remained the same when pulling c8ab881 on kartik-v:patch-4 into b20e576 on yiisoft:master.

Updated panel comments
@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage remained the same when pulling 8f1907c on kartik-v:patch-4 into b20e576 on yiisoft:master.

Updated Panel comment indentation for examples
@kartik-v
Copy link
Contributor Author

@kartik-v kartik-v commented Nov 24, 2013

@sv3tli0 the content has been made configurable for the bootstrap panel using an array.

@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage increased (+1.07%) when pulling 33b4349 on kartik-v:patch-4 into b20e576 on yiisoft:master.

kartik-v added 2 commits Nov 24, 2013
Updated and formatted comments
Corrected $content parameter in generatePanelTitle
@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage increased (+1.06%) when pulling 8ec1d73 on kartik-v:patch-4 into b20e576 on yiisoft:master.

Cleaned up variable calls in generatePanelTitle
@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage increased (+0.72%) when pulling e17e603 on kartik-v:patch-4 into b20e576 on yiisoft:master.

@coveralls
Copy link

@coveralls coveralls commented Nov 24, 2013

Coverage Status

Coverage increased (+0.72%) when pulling e17e603 on kartik-v:patch-4 into b20e576 on yiisoft:master.

@cebe
Copy link
Member

@cebe cebe commented Nov 24, 2013

Before you put more effort into this I'd like to clarify whether we actually need this class.
We had a discussion about the bootstrap helper before and came to the conclusion that it does not make sense to create a wrapper for every little feature that can be written in plain html more easily.

@yiisoft/core-developers opinions?

@samdark
Copy link
Member

@samdark samdark commented Nov 24, 2013

Personally plain HTML looks better to me than wrappers. Exception is when HTML is too complex to memorize.

@ghost
Copy link
Contributor

@ghost ghost commented Nov 24, 2013

My 2 cents: a wrapper only makes sense if it automatically leverages information managed by Yii. E.g. the fact that menu items in navbar are active according to the current URL, or a form field that can be highlighted on error. It seems to me that these are already available in the current Yii2-Bootstrap.

In Yii1 I used Yii Bootstrap (http://www.cniska.net/yii-bootstrap/) which wraps almost everything. While I liked it in general, some wrappers did not seem to make sense. In my reasoning, wrappers increase the load on the system (since PHP code needs to be evaluated), so I would only use them if they actually contribute...

@kartik-v
Copy link
Contributor Author

@kartik-v kartik-v commented Nov 25, 2013

Thanks for the inputs. I would like to extend the opinion by @samdark - which is logical. Note: these helpers address functionality specific to twitter bootstrap extension that is practically inbuilt into yii-2. I understand that in Yii-1.1 we did not have this (except maybe the JUI), so there is a change in mindset needed once we have bundled extensions with the core framework. How complete is the bundled extension's functionality is what I am trying to address.

Ok back to the great point by @samdark - that it is simpler to use HTML markup with a few exceptions. And these exceptions are exactly the considerations for using helpers when:

  1. The HTML markup is complex to remember, AND/OR
  2. A component is frequently needed in most programming situations, but the HTML markup is longer to write - or typing errors can be minimized by using a helper Html::icon('trash') vs span class="glyphicon glyphicon-trash".
  3. Corollary to the above. Have methods to allow changing a style parameter globally - whereby programmers can help theme applications when using helpers.
  4. Most importantly if the final markup to be generated - needs to process dynamic cases like using items from the database OR a configuration file OR process arrays OR code within a loop. For example using mediaList, listGroup, or panel or address.

Usage of these helpers are still optional and give programmers flexibility to use both. I have tried to shortlist bootstrap functionality which would need helper functions (and not tried to include every markup possible - check the constants in this class for example - they are simple markups which can be directly added).We maybe can work to trim few helpers down and eliminate some functions which seem useless. Even after this, if it is felt by @yiisoft/core-developers that this is not needed, I would still go with what the core team thinks. I would then leave this code for the community to use.

@mcd-php
Copy link
Contributor

@mcd-php mcd-php commented Nov 25, 2013

In my closed-source job project, i have Panel as widget with beginBody() method and separate $panelOptions,$headingOptions,$bodyOptions.

It can have heading as either option to ::begin() or as view content between ::begin() and ::beginBody(), body is only as view content.

The alterantive is maybe 3 widgets - Panel, PanelHeading, PanelBody, but this looks like an overkill.

Maybe i will opensource some pieces with boss discretion, but it's inferior in quality for OSS: under-documented, having some for-future pieces to be never completed.

Updated caret to display UP & DOWN options, with disabled status if needed. Content to jumbotron now can be supplied either as an array or string including buttons. Edits to abbr function and updated doc comments.
@coveralls
Copy link

@coveralls coveralls commented Nov 25, 2013

Coverage Status

Coverage increased (+0.27%) when pulling 3fa5bba on kartik-v:patch-4 into b20e576 on yiisoft:master.

Updated and formatted comments
@coveralls
Copy link

@coveralls coveralls commented Nov 25, 2013

Coverage Status

Coverage decreased (-0.08%) when pulling 7bcb4dc on kartik-v:patch-4 into b20e576 on yiisoft:master.

Updated address helper function
@coveralls
Copy link

@coveralls coveralls commented Nov 25, 2013

Coverage Status

Coverage remained the same when pulling c66cec4 on kartik-v:patch-4 into b20e576 on yiisoft:master.

Updated jumbotron buttons to include size
@coveralls
Copy link

@coveralls coveralls commented Nov 25, 2013

Coverage Status

Coverage increased (+0.27%) when pulling 5719de6 on kartik-v:patch-4 into b20e576 on yiisoft:master.

@kartik-v kartik-v mentioned this pull request Nov 29, 2013
@kartik-v
Copy link
Contributor Author

@kartik-v kartik-v commented Dec 4, 2013

Currently I am closing this PR, since I see this is not mission critical for the Yii milestone. I agree this is more bootstrap specific. I have released the code with additional helpers, as a separate extension for the community to access at yii2-helpers and a demo is available here. In case you feel otherwise, or any contribution is needed here from my end - please let know.

@kartik-v kartik-v closed this Dec 4, 2013
@kartik-v kartik-v deleted the kartik-v:patch-4 branch Apr 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.