Closed (fixed)
Project:
Configuration selector
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
28 Feb 2018 at 17:07 UTC
Updated:
21 May 2018 at 11:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
alexpottHere's a start. Needs tests though.
Comment #3
alexpottComment #4
daniel.bosenDo we need a new third party setting for a description? Right now, when configuring the a specific feature, I can not distinguish the given alternatives. The feature name is shown, but this is the same for all configs.
Comment #5
alexpott@daniel.bosen good point - I think we do need something. So people know what they are choosing between. Maybe we could try an use the view description and block admin description for example.
Comment #6
daniel.bosen@alexpott thats even better then adding a new description option, it would also be beneficial outside the selector UI to have those descriptions in good shape.
Comment #7
alexpottHere's #5 implemented. Unfortunately I still think we need a fallback because not all config ie blocks supports descriptions. But views do and it works great.
Also included are tests for the UI.
Comment #9
alexpottHmmm... let's use a bigger hammer.
Comment #10
mtodor commentedI have tested UI a bit. It looks nice and it makes a general idea more useful and transparent to the user - I like it.
There are few things that I have encountered. Maybe it's not directly related to UI, but it's easier to notice it now.
I have found just a few code nitpicks:
It would be nice to add logging for all configs where status is changed.
Can we use
$this->entityTypeManager()here?Nice one!
It would be nice to move
\Drupal::entityTypeManager()into variable.Since we are using
[@id="edit-table"]everywhere it would be better to use same selector here, just to reduce confusion. I think it's: "edit-table" and "edit-table--2".Comment #11
alexpottThanks for the review @mtodor!
Re nitpicks:
Going to address the other more important parts of #10 next.
Comment #12
alexpott@mtodor great questions in #10. Here's some thoughts. I'll start work on 1,2 and 5.
Comment #13
alexpottHere's points 1,2 and 5 addressed from #10.
For point 1 - I took a leaf out of the permissions UI (I know) and used colspan and indentation to group the entity types. I think it looks okay and only involves very very light CSS.

Comment #15
alexpottLet's make this pass regardless of the underlying Drupal 8.x version.
Comment #16
mtodor commentedI have tested changes and checked also code. It looks good to me and from my point of view, we can commit it.
P.S. There are few drupal coder styles that could be adjusted, but that can be done before commit.
Comment #17
chr.fritschCrediting @mtodor for reviewing
Comment #19
chr.fritschFixed