Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

EZP-25767: Implement the Everyone's Content dashboard block #589

Merged
merged 13 commits into from Jun 7, 2016
Merged

EZP-25767: Implement the Everyone's Content dashboard block #589

merged 13 commits into from Jun 7, 2016

Conversation

sunpietro
Copy link
Contributor

@sunpietro sunpietro commented May 13, 2016

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

TO DO:

  • create a new block view - Y.eZ.DashboardBlockAllContentView,
  • create a view for each row in a block - Y.eZ.DashboardBlockAllContentRowView,
  • add a block view to dashboard,
  • display row options after clicking on a row,
  • redirect to content location view,
  • redirect to content edit view,
  • JS unit tests

@sunpietro
Copy link
Contributor Author

ping @dpobel
I have some issues with displaying the loading state inside the block. Can you give me some suggestions on it?

@sunpietro
Copy link
Contributor Author

Another thing is that I would make more generic view for block view and block row view with tabular data that will be extended by other block views and block row views, but I'm afraid you don't not like this approach.

@dpobel
Copy link
Contributor

dpobel commented May 13, 2016

One comment about the block template: the reason I used divs instead of table is that row options were misaligned when trying to position them absolutely within a div inside the table cell. This approach solves that issue.

I did not look at the code yet, but that seems a bad reason. If you want to represent tabular data, just use a table. The behavior you add around that table should not imply changing anything on the table.

Another thing is that I would make more generic view for block view and block row view with tabular data that will be extended by other block views and block row views, but I'm afraid you don't not like this approach.

That's a bit early to do that. I don't even see the added value of having a view per row given at the moment the columns are defined and fixed. So this block could be very simple in terms of rendering ie it receives a list Location and Content Type and it displays that with the template.

new Y.eZ.DashboardBlockAllContentView({
priority: 1,
identifier: 'everyones-content'
}).addTarget(this).set('active', this.get('active'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should write:

new Y.eZ.DashboardBlockAllContentView({
    priority: 1,
    bubbleTargets: this,
});

the identifier is read only (and should be set in the code of eZ.DashboardBlockAllContentView itself. It's useless to forward the active flag, we know it's not active and there's an event handler that takes care of keeping active up to date.
it's not needed to set the active flag, we know the view is not active yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I didn't know about the bubbleTargets as it wasn't very well documented in YUI docs.

@dpobel
Copy link
Contributor

dpobel commented May 13, 2016

Alright now I read the whole patch.

First, for the list:

  • to answer, your first question, the easiest here is to add a loading class on the container when rendering the view without a list of contents and to remove that class when the views gets a list of contents.
  • there's no need for any view besides eZ.DashboardBlockAllContentView (the row view actually makes thing harder without bringing much value). It should generate a true table
  • as written in EZP-25767: Implement the Everyone's Content dashboard block #589 (comment), the loading work should be spread between the view service and the search plugin. Actually what does the view service depends on the answer to this comment. If we are interested only by the content items under "Content structure", then there you'll have to load the Location model for the rootLocation but if we really want to request all content items, then the starting is the virtual root and I guess you'll have to hardcode it in a similar way than this code (we'll see later how to unify that)

For the edit and views buttons appearing when clicking a row:

  • as written in EZP-25767: Implement the Everyone's Content dashboard block #589 (comment), you can not really use the button action view in that context, so for now you should just go for some buttons generated by the template.
  • to simplify that, just put those buttons in the last cell of the row and make it visible when the user taps on the row. (I know this won't play well with the potential scrollbar represented in the wireframe but for now there's not scrollbar...)
  • the last point also means you don't even have to do what is written in EZP-25767: Implement the Everyone's Content dashboard block #589 (comment), you can directly use a regular a element and generates the URL to edit or view with path in the template and the routing will be done by the browser.

criteria: {
SubtreeCriterion: this.get('rootLocation').get('pathString')
},
sortClauses: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one thing on that: this is not yet supported server side. Meaning the block will not display the modified content but a random list of content item. See https://jira.ez.no/browse/EZP-24315 (added it as a blocker).

@sunpietro
Copy link
Contributor Author

sunpietro commented May 16, 2016

First, for the list:

  • to answer, your first question, the easiest here is to add a loading class on the container when rendering the view without a list of contents and to remove that class when the views gets a list of contents.

thanks

I believe that keeping row views is a good idea. This way, the block makes a request for data and spreads the data into child views (row views) and renders them.
It would be too much if a block has to render the block container and then render the rows and populate them with data when data comes.
Having row views, also brings the potential for extending it in the future by adding more buttons and custom actions.
Do you still disagree with me?
I reverted the view template into clasic table.

  • the loading work should be spread between the view service and the search plugin. Actually what does the view service depends on the answer to this comment. If we are interested only by the content items under "Content structure", then there you'll have to load the Location model for the rootLocation but if we really want to request all content items, then the starting is the virtual root and I guess you'll have to hardcode it in a similar way than this code (we'll see later how to unify that)

This is why I wanted to have the root detection in the dashboard service, in my previous PR.

For the edit and views buttons appearing when clicking a row:

  • as written in EZP-25767: Implement the Everyone's Content dashboard block #589 (comment), you can not really use the button action view in that context, so for now you should just go for some buttons generated by the template.
    to simplify that, just put those buttons in the last cell of the row and make it visible when the user taps on the row. (I know this won't play well with the potential scrollbar represented in the wireframe but for now there's not scrollbar...)
  • the last point also means you don't even have to do what is written in EZP-25767: Implement the Everyone's Content dashboard block #589 (comment), you can directly use a regular a element and generates the URL to edit or view with path in the template and the routing will be done by the browser.

This makes me wondering, what is eZ.ButtonActionView for, if it's not for any kind of buttons?

@dpobel
Copy link
Contributor

dpobel commented May 17, 2016

I believe that keeping row views is a good idea. This way, the block makes a request for data and spreads the data into child views (row views) and renders them.

I'm not saying it's a bad idea, I'm saying it's not needed at this point.

It would be too much if a block has to render the block container and then render the rows and populate them with data when data comes.

this only adds 2 loops:

  • one to generate an array containing the result of toJSON on the Location and the ContentType
  • one in the template to loop over that array to display a table row per item.

Nothing really complicated.

Having row views, also brings the potential for extending it in the future by adding more buttons and custom actions.

AFAIK, this has not been asked anywhere. And doing it without the row views does not really prevent the extensibility. That's why at this point, the row views are not needed. If we need the row views, we could easily re-introduce them after without problems.

This is why I wanted to have the root detection in the dashboard service, in my previous PR.

and I did not say that was a bad code, I said, that was not needed at that point. Now it is needed, so it's time to re-introduce that.

This makes me wondering, what is eZ.ButtonActionView for, if it's not for any kind of buttons?

as written in the above comment, those were made to represent the button and the corresponding action in the bar views. Here, it's a different context, where we don't really want to act on the Location but more just navigate somewhere else, that's why regular links would work well for now and makes the whole thing a lot easier since we have everything in place to generate those links and the rest is done by the browser and the app without changing a single line.

@sunpietro sunpietro changed the title [WIP] EZP-25767: Implement the Everyone's Content dashboard block EZP-25767: Implement the Everyone's Content dashboard block May 18, 2016
@sunpietro
Copy link
Contributor Author

ping @dpobel

* @extends eZ.DashboardBlockBaseView
*/
Y.eZ.DashboardBlockAllContentView = Y.Base.create('dashboardBlockAllContentView', Y.eZ.DashboardBlockBaseView, [Y.eZ.AsynchronousView], {
events: EVENTS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I can remember, when using the asynchronous view extension, you can not define the dom event handlers this way otherwise the ones defined in asynchronous view (for the retry button) are completely overriden. You have to write it like

@sunpietro
Copy link
Contributor Author

ping @dpobel

name: 'eZ Dashboard Blocks View Service root location default value test',

setUp: function () {
this.rootLocationModel = new Y.Mock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed can be removed

@sunpietro
Copy link
Contributor Author

ping @dpobel

Y.Assert.areSame(contentJSON, params.items[0].content, 'Should provide content data of 1 item');
Y.Assert.areSame(contentTypeJSON, params.items[0].contentType, 'Should provide content type data of 1 item');
Y.Assert.areSame(locationJSON, params.items[0].location, 'Should provide location data of 1 item');
Y.Assert.areSame(contentInfoJSON, params.items[0].location.contentInfo, 'Should provide content info data of 1 item');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is incorrect, in this context, the point is to check that the contentInfo is directly available without using the Location (basically the result of https://github.com/ezsystems/PlatformUIBundle/pull/589/files#diff-b26270100a3f8b2b43dbc1f0c0ed3604R70)

So this should be written:

Y.Assert.areSame(contentInfoJSON, params.items[0].contentInfo, 'Should provide content info data of 1 item');

@sunpietro
Copy link
Contributor Author

ping @dpobel

@dpobel
Copy link
Contributor

dpobel commented Jun 3, 2016

+1
@StephaneDiot can you have a look please ?

@dpobel dpobel force-pushed the feature/ezp-25766_dashboard branch from bfd27c8 to 2459568 Compare June 6, 2016 09:29
@StephaneDiot
Copy link
Contributor

+1

@sunpietro
Copy link
Contributor Author

@dpobel this PR has just been rebased with feature branch

@dpobel dpobel merged commit b839ab8 into ezsystems:feature/ezp-25766_dashboard Jun 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
6 participants