Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In addition, we have no test coverage for the handling of disabled views, especially with the interaction of stale temp stores.
And, we have code that checks to see if a display is disabled, but that ignores the View's status in ViewEditFormController::getDisplayDetails()
Comment | File | Size | Author |
---|---|---|---|
#20 | 1921748-20.patch | 3.42 KB | damiankloip |
#11 | 1921748-11.patch | 4.07 KB | damiankloip |
#11 | interdiff-1921748-11.txt | 1.55 KB | damiankloip |
#4 | 1921748.patch | 2.54 KB | damiankloip |
Comments
Comment #1
dawehnerWe should probably asked the UI team about a general pattern in core?
Comment #2
tim.plunkettTagging.
Comment #3
tim.plunkettMaybe some from the usability team can provide guidance.
Comment #4
damiankloip CreditAttribution: damiankloip commentedTo start with I think it would make sense to include the status of the view in the actual edit form wrapper? I have also started adding tests with a basic addition to the page display test, to make sure that a disabled view returns a not found.
Comment #5
dawehnerI'm wondering whether 'enabled' is specific enough for a css class?
Comment #6
damiankloip CreditAttribution: damiankloip commentedNot sure, you have the other classes too though - so I think that's OK. See what people think I guess :)
Comment #7
dawehnerYeah we can change it later, if needed. Do you think we should add a test for that bit?
Comment #8
Bojhan CreditAttribution: Bojhan commentedCan this get a screen?
Comment #9
dawehnerWell, we still need some css to show anything :)
Comment #10
damiankloip CreditAttribution: damiankloip commentedI'm going to ask a ux person about this today.
Comment #11
damiankloip CreditAttribution: damiankloip commentedAdded some tests for the actual class too.
Do you think we should get this in now? The logic will then be completed, and another issue can just deal with the UX/theme side of this? This seems like a sensible step.
Comment #12
tim.plunkettI agree that a design discussion should happen in a separate issue, maybe even one each for Seven and Bartik.
At the least, this will allow contrib to use this class.
And it allows us to add test coverage.
Comment #13
Bojhan CreditAttribution: Bojhan commentedSure, I see no reason not just to add it here though :P. It seems like we could do it quite easily.
Comment #14
Bojhan CreditAttribution: Bojhan commentedwhoops
Comment #15
tim.plunkettAdd what here? No one has proposed any actual UI improvements. I can implement any design you throw at me, but I am not qualified to invent one :)
Comment #16
damiankloip CreditAttribution: damiankloip commentedYeah, we are not doing any cosmetic changes here now.
Comment #17
Bojhan CreditAttribution: Bojhan commentedOk, let me know when the followup is created and I will whip up a mockup
Comment #18
damiankloip CreditAttribution: damiankloip commented@Bojhan, here we go: #2001094: Design and implement style changes to signify disabled views on the Views UI edit form
Comment #19
alexpottThe comments here make no sense... first you access a disabled view and then you disable the view and test again...
I think what you mean is
Test accessing a disabled page for a view.
Comment #20
damiankloip CreditAttribution: damiankloip commentedRerolled, and reworked the DispayPageTest assertions to fit with Daniels shiny new route based tests.
Comment #21
dawehnerThat is better now!
Comment #23
damiankloip CreditAttribution: damiankloip commented#20: 1921748-20.patch queued for re-testing.
Comment #24
dawehnerThis time it passed
Comment #25
alexpottCommitted 96c82a2 and pushed to 8.x. Thanks!