 |
@DenisChenu Time to review? |
|
|
Not sure how to make CMenu play well with Bootstrap. Edit: Answer: htmlOptions => [class => btn] Edit 2: But HTML structure is different than CMenu. Edit 3: Can use item templates for this. ^^ |
|
|
Example (unfinished) implementation using CMenu: ``` $this->widget('zii.widgets.CMenu', [ 'htmlOptions' => [ 'class' => 'nav navbar-nav scoped-topbar-nav ls-flex-item ls-flex-row grow-2 text-left' ], 'items' => [ [ 'label' => 'Activate survey', 'url' => ['site/index'], 'template' => '<div class="topbarbutton">{menu}</div>', 'linkOptions' => ['class' => 'btn btn-primary topbarbutton' ] ], [ 'label' => 'Preview survey', 'url' => ['product/index'], 'template' => '<div class="topbarbutton"><div class="btn btn-default"><i class="fa fa-cog"> {menu}</div></div>', 'items' => [ [ 'label' => 'New Arrivals', 'url' => ['product/new', 'tag'=>'new'] ], [ 'label' => 'Most Popular', 'url' => ['product/index', 'tag' => 'popular'] ], ] ], [ 'label' => 'Login', 'url' => ['site/login'] ] ] ]); ``` Problems: How to implement Bootstrap HTML structure? How to replace anchor with button tag for dropdown menu? |
 |
I like CMenu. I haven't used it in a while. I would go for option 3. Still some comments. `Use raw HTML (forwardporting LS3`. Keep in mind that file is not just for rendering. Permissions are checked. So, we will need an extra component that will set the menu-item-array according to the action and the user profile. Maybe could be a base controller method linked to a helper class? Maybe it could be an extension from CMenu that receives the action and the user as parameters? (hooking from beginContent) `Extend CMenu Yii widget and use objects instead of associative arrays to define the menu data` I believe we could handle that on later stages. What would be the differences between objects and arrays? arent' both sent as reference (if remember correctly)? `As 4, but extend CWidget (CMenu does not contain much that we need, and doesn't use view files)` We can use itemTemplate (https://www.yiiframework.com/doc/api/1.1/CMenu#itemTemplate-detail) and getViewFile (https://www.yiiframework.com/doc/api/1.1/CWidget#getViewFile-detail). Honestly, we would need to run a PoC first, but I am very confident this works well with bootstrap. I even remember the default app from Yii uses bootstrap. As for the question selector, we could use a custom rendered and in there make a query for fetching the questions. Then we would minimize the data passed as parameters. I would dig a little bit more on the cMenu and look for examples with permission checking. |
|
|
CMenu test here: https://github.com/LimeSurvey/LimeSurvey/commit/1f29debbd7fb03635b1f1140789dc8691d3b9533 Someone will have to try to get this to work. Otherwise, I vote against. |
|
|
> What would be the differences between objects and arrays? I should write an article about this, but with objects you can do: * Validation * Default values * Inheritance, interfaces, polymorphism * Immutability * Encapsulation * Split up in files * Self-documentation and documentation * Type-safety ("tag" the data you're passing around, expressing intent more clearly) There's no performance difference between arrays and objects in PHP. Arrays have value semantics, objects are passed as references. |
|
|
"PHP: Use associative arrays basically never": https://steemit.com/php/@crell/php-use-associative-arrays-basically-never > In just about every other situation I can think of, named classes win. Their memory usage is half that of a corresponding array. The optimizations the engine can do when it knows up front what the structure of your data is going to be are massive and pay off huge dividends in memory consumption. They're also over 10% faster. The only downside is when trying to serialize them when there is an added cost to time, memory, and stored size. When we also consider that a classed object is far more self-documenting than an associative array, gives IDEs the ability to auto-complete for you, and gives you a place to include additional documentation (which you should include), it's one of the clearest wins I've seen in PHP. But also: > The first thing we can conclude is that if the one and only thing you care about is serialization/deserialization performance, associative arrays still win. They're the most time efficient by more than 50%, and the most space efficient by up to 20%. |
 |
I don't really care on the final solution ;) But something that can be extendable … or user manageable … Create our own widget (or extende CMenu) based on https://github.com/LimeSurvey/LimeSurvey/tree/master/application/libraries/MenuObjects ? |
|
|
Yes, my widget uses the same classes as the ones for the plugins - Menu and MenuItem (and others inherited from those). |
|
|
@gabrieljenik Can you push the code you made, too, using CMenu? So it can be reviewed. :) |
|
|
``` $this->widget('zii.widgets.CMenu', [ 'htmlOptions' => [ 'class' => 'nav navbar-nav scoped-topbar-nav ls-flex-item ls-flex-row grow-2 text-left' ], 'items' => [ [ 'label' => 'Activate survey', 'url' => ['site/index'], 'template' => '<div class="topbarbutton">{menu}</div>', 'linkOptions' => ['class' => 'btn btn-primary topbarbutton' ] ], [ 'label' => 'Preview survey', //'url' => ['product/index'], 'template' => '<button class="btn btn-default dropdown-toggle" type="button" data-toggle="dropdown"><i class="fa fa-cog"> {menu} <i class="caret icon"></button>', 'submenuOptions' => [ 'class' => 'dropdown-menu' ], 'itemOptions' => [ 'class' => 'topbarbutton dropdown' ], 'items' => [ [ 'label' => 'New Arrivals', 'url' => ['product/new', 'tag'=>'new'] ], [ 'label' => 'Most Popular', 'url' => ['product/index', 'tag' => 'popular'] ], ] ] ] ]); ``` image.png (12,041 bytes) |
|
|
Nicely done, Gabriel! However, one problem I see with this approach is that HTML ends up in strings among programming logic. Is it possible to move `template` to a view file, for example? HTML in strings have drawbacks compared to HTML in view files, e.g. lacking syntax highlight, indentation. In the long run, I believe this hurts maintainability and readability. Another alternative would be to have a new class "CMenuItem` that implements ArrayAccess interface. This way, we could get rid of long, associative arrays. |
|
|
> Is it possible to move template to a view file, for example? :+1: It can be really great :) |
|
|
@ollehar, Yes this need improvement. We were just doing it as a Proof of Concept for the dropdowns. So, netx step is separating html and logic as much as possible. |
|
|
@gabrieljenik The thing is, as you work through this, I think in the end you will re-implement the solution I already did with CWidget. :) Which has classes and view files. |
|
|
Well... that could be true... :) Not sure how much advanced is that solution, but both go in the same direction. So you let me know which direction we take... |
|
|
We'll discuss in the team and let you know. Thanks! |
|
|
Seems like we can't agree right now, so we'll go back to LS3 style, with buttons in view files. No widget. |
|
|
Good. Will adapt question groups toolbars and others from the screens I ported |
|
|
Related discussion of my attempt: https://forum.yiiframework.com/t/renderobjectasviewwidget/130644 |
|
|
Notes from Patricia: We decided to use the topbar from LS3 (in conception meeting) in first step. These are the relevant "new" things in this regard: in an action there must be the line: $aData['renderSpecificTopbar'] = 'copyQuestiontopbar_view'; The value 'copyQuestiontopvar_view' is the name of the view where the specific topbar is saved. This topbar is rendered directly in the view $this->render('copyQuestionForm', $aData); by using the function renderPartial('topbars/'.'nameOfTopbarView', $data)The view for the topbar is in the folder views/controllerName/topbars (e.g. /views/questionAdministration/topbars/copyQuestiontopbar_view). So every controller can have more then one topbar (depending on the action). Of course some actions can also use the same topbar if needed. That prevents using always "if...else". Advantages: -- no special call to topbar function anymore in layout_questioneditor (except those we still have now) -- topbars could always be searched&found at the same place (views/controllerName/topbars/sometopbar.php) -- buttons can be added easily -- permissions can be checked in the controllers action directly (no permission to use the action --> no view is rendered --> no buttons are shown) |
|
|
Any branch where we can see an example? |
|
|
Ping @pstelling |
 |
it's in the branch "https://github.com/LimeSurvey/LimeSurvey/tree/task/zoho-L41-T55-topbar-copy-question" ping @gabrieljenik |
|
|
Hi @pstelling Thanks for the code. Please, look at the attachment. This is how it looks when the page is loaded through the sidebar w/pjax. I think we need to work on layout for some pages (I think it is the same layout for survey details) and remove the topbar from it so the top bar is loaded by pjax only (second toolbar), right? Also, some styling needs to be twitched, right? On the second screenshot we can see everything makes sense as the layout is different. Thoughts? Should we go ahead with it? image-2.png (20,727 bytes) image-3.png (33,198 bytes) |
|
|
Hi Gabriel, thanks for the hint. Yes obviously there is a problem when rendering something with Ajax Request. Did you change/implement the topbar in "Edit email templaztes"? I'll check this. Maybe we will discuss again about topbar solutions. Bye Patricia |
|
|
Yes, for email templates we created a top bar, copying it from LS3. I agree, not the best one :) We could actually add some stuff on it. But that's enhancements. Look forward to your comments on how to move forward. I guess changing some layouts is involved. But will wait for your comments. Thanks |
|
|
Hi Gabriel, thanks for your comment :). We had the idea to stay with the "design" (layout) for the topbar like it is in LS3. Would you like to have another design/layout for it? What would you prefer? What kind of "stuff" would you like to add? Enhancements are always good. Best when we get information about it. Would you like to write it (e.g. Buttons missing or would be nice to have the Button X ). Would also be nice to know the "intention" behind, like "having this button in addition gives us the possibility of ...". Of course there should not be two topbars (one behind the other, like in image2). Can you send me the link to the branch where you implemented the topbar for email templates? Thanks a lot for all the information you give us in this regard. I'm looking forward to hear from you. Best regards Patricia |
|
|
> What kind of "stuff" would you like to add? Enhancements are always good. Best when we get information about it. Would you like to write it (e.g. Buttons missing or would be nice to have the Button X ). Would also be nice to know the "intention" behind, like "having this button in addition gives us the possibility of ...". I was referring to the email template toolbar, not the general toolbar system > Of course there should not be two topbars (one behind the other, like in image2). Can you send me the link to the branch where you implemented the topbar for email templates? Sorry, I missed the link on this ticket. https://github.com/LimeSurvey/LimeSurvey/pull/1634 Let me know if you want to hook into a call. Thanks! |
|
|
Added some code here: https://github.com/LimeSurvey/LimeSurvey/pull/1648 Don't kill me :) I have taken the concept a little bit further as: - some topbars need to be shared among views (they are the same) - need some common code in between all topbars as to reuse code and reduce maintenance efforts. Still, everything is overridable, so if we need a specific topbar for a single view, we can do that. I integrated it with the LSBaseController and the new rendering mechanisms. Double topBars is solved. This a WIP, I am working on the surveyTopBar which is very general. |
|
|
> some topbars need to be shared among views (they are the same) +1, but i don't like `<?= $rightSideContent ?>` I prefer params, @patricia start something here : https://github.com/LimeSurvey/LimeSurvey/blob/develop/application/views/admin/super/fullpagebar_view.php I use it in views (and adapt send button) https://github.com/LimeSurvey/LimeSurvey/pull/1632/files#diff-826c61128100ca4f347af88c674f69e38d2427d3fe6323cfe39807444e01d958R20 |
|
|
> I prefer params, @patricia start something here : https://github.com/LimeSurvey/LimeSurvey/blob/develop/application/views/admin/super/fullpagebar_view.php Those kind of params are something to come. Definitely. That `$rightSideContent` is rendered by a view whose name is sent by a parameter. That rightSideContent redended can be on top of a blankTopBar or on top of a shared top bar with some default buttons already. |
|
|
I think it's totally related to the Menu project from @ollehar too. Finally : all top tools are menu … |
|
|
I think it is the same! Actually, I understand that project concluded into this. |
|
|
Turned configuration into a class and rendering enclosed into a widget: https://github.com/LimeSurvey/LimeSurvey/pull/1648 |
|
|
Merged to DEV. I think this can be closed now. |
|
|
Some extra comments: On LS3 ckeditor configuration wrapper and utilities were coded in a way that sometimes the textarea (and control) was fecthed by name and sometimes by id, using a single value for fecthing using name or id criteria. Something like GetByName(YXZ) or GetById(YXZ). On LS4 textareas can have different name and id. Which is good. Now the wrapper was fixed as to get only elements by Id. |
- Anonymous
