EZP-25552: Explorer view #742

StephaneDiot · 2016-11-29T14:59:22Z

jira : https://jira.ez.no/browse/EZP-25552

Description

This is the first version of the content repository finder explorer. This should allow to browse content with an explorer.

Screencast

https://youtu.be/vMsBmqIgLeU

TODO

feature
unit tests
CSS

StephaneDiot · 2017-01-05T14:11:14Z

@dpobel @yannickroger ready for review (CSS missing)

dpobel · 2017-01-09T10:25:53Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoverybrowseview.js

@@ -22,6 +26,7 @@ YUI.add('ez-universaldiscoverybrowseview', function (Y) {
     */
    Y.eZ.UniversalDiscoveryBrowseView = Y.Base.create('universalDiscoveryBrowseView', Y.eZ.UniversalDiscoveryMethodBaseView, [], {
        initializer: function () {
+            console.warn('UniversalDiscoveryBrowseView is deprecated');


since the whole module is deprecated, you don't need to add this console.log call

dpobel · 2017-01-09T10:26:50Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+     * @extends eZ.TemplateBasedView
+     */
+    Y.eZ.UniversalDiscoveryFinderExplorerLevelView = Y.Base.create(
+        'universalDiscoveryFinderExplorerLevelView', 


cs (if the line is too long, just put the name in a variable with a shortname)

dpobel · 2017-01-09T10:33:01Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+                    location: item.location.toJSON(),
+                    contentInfo: item.location.get('contentInfo').toJSON(),
+                    contentType: item.contentType.toJSON(),
+                    content: item.content.toJSON(),


it does not seem to be used in the corresponding template. In addition, you already pass the content info so the Content is not needed.

dpobel · 2017-01-09T10:33:18Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+                itemsJSONified.push({
+                    location: item.location.toJSON(),
+                    contentInfo: item.location.get('contentInfo').toJSON(),
+                    contentType: item.contentType.toJSON(),


not used any where as well

dpobel · 2017-01-09T10:38:38Z

Resources/public/templates/universaldiscovery/explorerlevel.hbt

@@ -0,0 +1,24 @@
+<div class="ez-ud-finder-explorerlevel-content ez-asynchronousview">
+    {{#if loading}}


instead of passing loading and adding a markup when the view is in loading module, I would make sure the view add/remove a class on its container depending on its loading state.

dpobel · 2017-01-09T10:54:56Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+                        this.fire('explorerNavigate', {
+                            data: item,
+                            location: item.location,
+                            depth: this.get('depth')


there's no depth attribute

dpobel · 2017-01-09T10:56:40Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+        initializer: function () {
+            this.after('activeChange', function () {
+                if (this.get('active')) {
+                    Y.Array.each(this.get('levelViews'), function (levelView) {


this is exactly the wakeUp method

dpobel · 2017-01-09T11:04:13Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+                    }, this);
+                }
+            });
+            this.on('*:explorerNavigate', function(e) {


this event handler deserves its own method with a good name

dpobel · 2017-01-09T11:09:09Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+                    this._addLevel(e.location);
+                }
+                Y.Array.each(this.get('levelViews'), function (levelView) {
+                    this._renderLevelView(levelView);


this is needed only for the newly added levels not for all of them

dpobel · 2017-01-09T11:10:08Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+            var LevelView = this.get('levelViewConstructor'),
+                levelView = new LevelView({
+                    parentLocation: location,
+                    depth: this.get('levelViews').length + 1});


StephaneDiot · 2017-01-18T08:53:01Z

@dpobel rdy for review

dpobel · 2017-01-18T10:47:03Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+                this._removeLevels(count);
+            }
+            if (this.get('levelViews').length > 1) {
+                this.get('levelViews')[this.get('levelViews').length - 2].removeHighlighting();


not super fan of this removeHighlighthing method. Since behind the scene it actually set an attribute, why not directly update the attribute here ?

dpobel · 2017-01-18T10:49:15Z

Resources/public/templates/universaldiscovery/explorerlevel.hbt

+                <li class="ez-explorer-level-list-item ez-font-icon
+                    {{#if location.childCount}} is-parent-location{{/if}}
+                    {{#if selectedLocationId}} is-selected{{/if}}
+                    {{#if ../ownSelectedItem}} has-selected-item{{/if}}"


related to my previous remark on removeHightlighting method. Given it's an attribute of the view, this should probably mean that a class is added or removed on the container when the attribute is updated

dpobel · 2017-01-18T10:51:27Z

Resources/public/css/theme/views/universaldiscovery/selected.css

@@ -3,6 +3,10 @@
 * For full copyright and license information view LICENSE file distributed with this source code.
 */

+.ez-view-universaldiscoveryselectedview {
+    margin-left: 1em;


in theory, you are just reusing the selected view in the Finder so no CSS change should be needed for the component. This also means there's a regression risk in others method using the Selected view.
(also setting a margin should not be a theme CSS)

dpobel · 2017-01-18T10:52:38Z

Resources/public/css/views/universaldiscovery/explorer.css

@@ -0,0 +1,16 @@
+.ez-view-universaldiscoveryfinderexplorerview .ez-ud-finder-explorerlevel {
+    height: 100%;
+    display: -webkit-box;


we don't need the prefixes on flexbox anymore so you can stick with the standard properties:

display: flex; flex-direction: row;

dpobel · 2017-01-18T10:54:57Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+         * @protected
+         * @return {Object} Returns an item containing a location.
+         */
+        _findLocationInItems: function (locationId) {


if it returns an item, it can not be named _findLocation.
_findItemByLocationId maybe ?

dpobel · 2017-01-18T11:00:34Z

Resources/public/templates/universaldiscovery/explorerlevel.hbt

+                    {{#if selectedLocationId}} is-selected{{/if}}
+                    {{#if ../ownSelectedItem}} has-selected-item{{/if}}"
+                >
+                    <a class="ez-explorer-level-item ez-font-icon{{#if selectedLocationId}} is-selected{{/if}}"


given you set add an ellipsis when that's too long, you should add a title attribute with the full name

dpobel · 2017-01-18T11:01:23Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+         */
+        _handleLevelViews: function (depth, location) {
+            if (depth < this.get('levelViews').length) {
+                var count = this.get('levelViews').length - depth;


CS: var at the beginning of the method

dpobel · 2017-01-18T11:01:42Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerview.js

+         */
+        reset: function (name) {
+            if (name == 'levelViews') {
+                var count = this.get('levelViews').length - 1;


CS: var at the beginning of the method

dpobel · 2017-01-18T11:02:37Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+     * @constructor
+     * @extends eZ.TemplateBasedView
+     */
+    Y.eZ.UniversalDiscoveryFinderExplorerLevelView = Y.Base.create( viewName, Y.eZ.TemplateBasedView, [Y.eZ.AsynchronousView], {


CS: no space before viewName

dpobel · 2017-01-18T11:07:36Z

Resources/public/js/views/universaldiscovery/ez-universaldiscoveryfinderexplorerlevelview.js

+            }, this);
+            container.setHTML(this.template({
+                items: itemsJSONified,
+                loading: this.get('loading'),


not needed anymore, no ?

StephaneDiot · 2017-01-19T14:52:56Z

Ready for review @dpobel @yannickroger

dpobel

did you fix the finder inconsistent reset when switching to another method after using it and getting back to the finder ?

dpobel · 2017-01-20T08:39:04Z

Resources/config/css.yml

@@ -127,6 +130,9 @@ system:
                - 'bundles/ezplatformui/css/theme/views/universaldiscovery/browse.css'
                - 'bundles/ezplatformui/css/theme/views/universaldiscovery/search.css'
                - 'bundles/ezplatformui/css/theme/views/universaldiscovery/selected.css'
+                - 'bundles/ezplatformui/css/theme/views/universaldiscovery/finder.css'
+                - 'bundles/ezplatformui/css/theme/views/universaldiscovery/explorer.css'


it seems to be empty so it can be remove from here and from the filesystem

dpobel · 2017-01-20T08:40:08Z

Resources/public/css/views/universaldiscovery/finder.css

@@ -0,0 +1,32 @@
+.ez-view-universaldiscoveryfinderview {
+    /*margin: 0 -1em -1em -1em;*/


should be removed

StephaneDiot · 2017-01-20T09:00:21Z

I did, it now keep the state of the explorer when switching method and coming back to the explorer ( as shown in the screencast )

dpobel

ah I did not notice the screencast :) so besides the last minor comments, +1
good job :)

yannickroger · 2017-01-20T14:43:23Z

Resources/public/css/views/universaldiscovery/selected.css

@@ -16,8 +16,9 @@
    min-height: 0;
 }

-.ez-view-universaldiscoverybrowseview .ez-ud-pane-selected {
-    width: 25%;
+


nitpick: empty line, there's also another one bellow.

yannickroger · 2017-01-20T14:55:45Z

Resources/public/templates/universaldiscovery/explorerlevel.hbt

+                        {{ contentInfo.name }}
+                    </a>
+                </li>
+


nitpick again

yannickroger · 2017-01-20T14:58:23Z

Don't forget to squash the commits :)

StephaneDiot force-pushed the EZP-25552_Explorer_View branch 2 times, most recently from 111b9fa to 17c1d22 Compare January 5, 2017 10:25

StephaneDiot changed the base branch from feature/EZP-25552_UDW_Finder_Content_Explorer to master January 5, 2017 10:27

StephaneDiot changed the title ~~WIP [Paused] EZP-25552: Explorer view~~ EZP-25552: Explorer view Jan 5, 2017

StephaneDiot force-pushed the EZP-25552_Explorer_View branch from 17c1d22 to e458fac Compare January 5, 2017 10:36

StephaneDiot changed the base branch from master to feature/EZP-25552_UDW_Finder_Content_Explorer January 5, 2017 13:26

StephaneDiot force-pushed the EZP-25552_Explorer_View branch 2 times, most recently from c7a2331 to bf2970d Compare January 5, 2017 14:08

dpobel suggested changes Jan 9, 2017

View reviewed changes

StephaneDiot force-pushed the EZP-25552_Explorer_View branch 3 times, most recently from 418f0e6 to 3bbb411 Compare January 13, 2017 14:17

dpobel suggested changes Jan 18, 2017

View reviewed changes

StephaneDiot force-pushed the EZP-25552_Explorer_View branch from fa50a0d to 3e3f651 Compare January 19, 2017 14:46

StephaneDiot changed the base branch from feature/EZP-25552_UDW_Finder_Content_Explorer to master January 19, 2017 14:49

StephaneDiot changed the base branch from master to feature/EZP-25552_UDW_Finder_Content_Explorer January 19, 2017 14:52

StephaneDiot force-pushed the EZP-25552_Explorer_View branch from 3e3f651 to 3bd5ea4 Compare January 19, 2017 15:27

dpobel reviewed Jan 20, 2017

View reviewed changes

dpobel approved these changes Jan 20, 2017

View reviewed changes

StephaneDiot force-pushed the EZP-25552_Explorer_View branch from 3bd5ea4 to 64210b5 Compare January 20, 2017 10:17

yannickroger approved these changes Jan 20, 2017

View reviewed changes

EZP-25552: UDW Finder Explorer

1af55aa

StephaneDiot force-pushed the EZP-25552_Explorer_View branch from 64210b5 to 1af55aa Compare January 26, 2017 12:47

StephaneDiot merged commit 0525fe7 into feature/EZP-25552_UDW_Finder_Content_Explorer Jan 26, 2017

yannickroger deleted the EZP-25552_Explorer_View branch February 6, 2017 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-25552: Explorer view #742

EZP-25552: Explorer view #742

StephaneDiot commented Nov 29, 2016 •

edited

Loading

StephaneDiot commented Jan 5, 2017 •

edited

Loading

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

dpobel Jan 9, 2017

StephaneDiot commented Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

dpobel Jan 18, 2017

StephaneDiot commented Jan 19, 2017 •

edited

Loading

dpobel left a comment

dpobel Jan 20, 2017

dpobel Jan 20, 2017

StephaneDiot commented Jan 20, 2017

dpobel left a comment

yannickroger Jan 20, 2017

yannickroger Jan 20, 2017

yannickroger commented Jan 20, 2017

		@@ -0,0 +1,24 @@
		<div class="ez-ud-finder-explorerlevel-content ez-asynchronousview">
		{{#if loading}}

		@@ -0,0 +1,32 @@
		.ez-view-universaldiscoveryfinderview {
		/margin: 0 -1em -1em -1em;/

EZP-25552: Explorer view #742

EZP-25552: Explorer view #742

Conversation

StephaneDiot commented Nov 29, 2016 • edited Loading

Description

Screencast

TODO

StephaneDiot commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDiot commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDiot commented Jan 19, 2017 • edited Loading

dpobel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDiot commented Jan 20, 2017

dpobel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickroger commented Jan 20, 2017

StephaneDiot commented Nov 29, 2016 •

edited

Loading

StephaneDiot commented Jan 5, 2017 •

edited

Loading

StephaneDiot commented Jan 19, 2017 •

edited

Loading