Split the Views UI listing into two tables for enabled and disabled

I was always a little surprised Views did this, and why it didn't have two separate tables. Keep in mind though, that you add a empty table pattern if there are by default no views active (although I assume we fix this before release).

Also, whats the function of "add from template", I always thought that is like a browser but I know its not :D

Adding tags.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -150,10 +153,25 @@ public function buildOperations(EntityInterface $entity) {
+        '#empty' => t('There is no @label yet.', array('@label' => $this->entityInfo['label'])),

This is the only part I don't like, but this came from the base class anyway, and we don't have plural entity names. So not sure if this can be imperoved yet.

Ahhh

Needs review from others :)

The visual split is great. +1 from me.

We need a little spacing below the table so it doesn't look so condensed.

1. I like the split as well (I came to the same suggestion in http://drupal.org/node/1806022#comment-6685112)
2. Agreed with bojhan, an additional 10px between first table and the 'Disabled' header would be helpful.
3. How does it work now when you enable a view in the bottom table. Is it added to the top at the first table? Any visual clues that something changed? (I'd check, but my local setup is borked atm). Same for the reverse.

Sounds like it needs at least some tweaking

This looks good to me :)

Yoroy his question has a disguised nice suggestion, which wouldn't really be views related. But to have a "background effect" when you enable something. So you clearly see where it is, since we don't do like an anchor?

I cant put it RTBC since it prolly needs markup review.

The patch looks great to me, as does the screenshot. Can we add some simple test coverage for it?

We should also probably do the accessibility review checks from the gate. I understand the use of a header vs. a caption, but we don't have an <h2> on the page that I'm aware, so shouldn't they be <h2>?

A definite improvement. Also tagging that this came out of the BADCamp study

I'll try to review this patch as soon as I can.

XJM in #14 is definitely right: headers should be "h2"...

Yep, I think this is looking awesome, and yes, H3 is good. Are we ready to go with this? I would RTBC but not sure if we need to wait on usability?

This is good from UX perspective.

The UX is fine. xjm suggested adding tests for this, I guess that's a needs-work then.

I'll write some tests for this, I think all of our listing tests got moved into the base Entity listing tests.

Hmm, I'm really not sure <h3> is okay. Making the assumption that <h2> is a block title elsewhere on the page only makes it more broken -- it would make these sections logical children of those blocks? So it seems pretty confusing and I don't think "the header cascade is broken elsewhere" is a good reason to break it here as well. Maybe someone like mgifford or Everett could weigh in on this?

More explicitly, a broken header cascade will probably introduce an accessibility regression for #1806288: Fix duplicate ID's in Views HTML structure. Let's test it using that tool.

OK, sorry I hadn't had time to reply before. I think that the header cascade shouldn't be broken.

Let me give an example according to WCAG standards. Using an "h2" heading means the same as paragraph number "1.2". Using an "h3" heading means the same as paragraph number "1.2.1". So the question is obvious: how can we say paragraph number "1.2.1" if we do not have a parent paragraph ("1.2")? What's the parent of our two tables? Aren't they two blocks (meaning "piece of content")?

However I have a question: how will we deal with enabling/disabling a view after splitting the current table into two different ones? Where will the cursor be placed after enabling/disabling a view?

Ok, H2 it should be then.

@falcon03: do you have a suggestion on where you would expect the cursor to be after an enable/disable action?

Should it go along with the view you've moved or stay in the area you removed that view from? Should enable behave differently than disable?

I can imagine you might want to move the cursor along with the view you enabled because you are likely wanting to edit it further. When disabling a view you might want to stay in the enabled views list because you don't want to do anything further with the disabled view. That would mean that the cursor would always end up somewhere in the enabled views listing. Makes sense?

@yoroy: if you could implement your approach for enabling/disabling a view it would be really wonderful!

Maybe we should document this behavior before the two tables so that blind people can easily know what to expect. If we want to hide this information from sighted people we can assign to this text the "element-invisible" css class.

Since this could introduce an important new pattern in Drupal (let's think to the modules page), IMO this seems a very interesting topic for the A11y sprint on next Sunday, though...

I can implement nothing of the kind because I'm not a coder, but it seems you think the idea is worth exploring.

I'm not sure the same pattern would be applicable to the modules page (yet) because enabling or disabling a module doesn't necessarily move that module to a different place in the list. It would be relevant to the 'Manage display' page of a content type in Field UI, I took note of that in http://drupal.org/node/1770720#comment-6698546

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -156,10 +159,27 @@ public function buildOperations(EntityInterface $entity) {
+        '#empty' => t('There is no @label yet.', array('@label' => $this->entityInfo['label'])),

The @label here needs to be "enabled foo" or "disabled foo" I think.

Actually, more idiomatic text would be "There are no enabled views" or "There are no disabled views." Do we have pluralization for labels? :)

Looks great and agree with xjm that we should write that the empty text as she suggests.

LOL 'there-is-no-Dana-only-View'

Agreed, "yet" adds an assumption that is likely to be false. (Thence #38).

This looks great to me now, both from a code review and from testing it manually. I love the way it updates when we toggle the enable/disable. I also tested with the keyboard only for accessibility to ensure that the cursor position makes sense when enabling/disabling. (In FF since webkit browsers cursor position is broken anyway in webkit; #1529814: Fix skiplink behavior for Webkit browsers.)

I did notice that it's hard to see when the focus is around the dropbutton (and actually impossible to see when focus is on the drop arrow in FF), but I'll follow up about that in the dropbutton followup issue.

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -156,10 +159,30 @@ public function buildOperations(EntityInterface $entity) {
+    // @todo Replace with a plural entity label.
+    $plural_label = 'Views';

Oh one thing. I think this needs to be translated too, as the label is normally translated?

And actually I realize we can't just shove a globally defined plural label into the string, because the word isn't pluralized in this context in all languages. (Some languages use the singular or even a different case with a "there are no X" construction.) So I think we'd have to use format_plural(), and then it gets even more complicated...

Maybe we shouldn't try to be so clever and instead just translate the whole string? That gives translators the context they need, and avoids the sticky question. We can re-address the question if we extend this functionality to other entities following #1826602: Allow all configuration entities to be enabled/disabled.

I'll ask for feedback from the D8MI folks.

This is the Views controller, so I'm not sure why would we want to make 'Views' a variable here. Do we expect others to extend from this and have things that are not Views be controlled? To me that sounds like a far fetched idea, but maybe you have a use case?

I agree with xjm the best would be to just have the whole sentence in one string, which would let translators to use the right wording. Otherwise they would just work around this, remove the placeholder and put in 'Views' in their translations :D

Thanks @Gábor Hojtsy! Regarding why we'd use a variable in the first place, I infer that @tim.plunkett's goal was providing something that could be abstracted for the list controller for #1826602: Allow all configuration entities to be enabled/disabled. I'll reference this issue over there.

I'm not sure that is related to that issue :)

@damiankloip, it's related because we will need listings like the Views one for those entities as well if we abstract the property.

Ok fair enough. It's more about having a plural label than anything else I guess.

Patch and tests look good to me!

Committed and pushed to 8.x. Thanks!