Problem/Motivation
In order to implement the new create content design (screenshot of design included), the vertical tabs used for settings fields on node forms need to be re-done as a set of collapsibles. These will ultimately be rendered within a two-column layout, but this issue focuses on implementing the change from vertical tabs to collapsibles.
Current status of this patch looks like this:
Proposed resolution
It would be nice to implement this using #1168246: Freedom For Fieldsets! Long Live The DETAILS., however at this time before feature freeze we should not reply on that, but use Core's existing collapsible form elements. Styling does not have to exactly match the proposed design at this point. The focus of this issue should be getting the feature working, looking good in Stark and acceptable in Seven. The two-column layout will be done in #1838156: Implement the two-column layout for the new create content page. The polish to match the design will be applied in #1751754: Implement new form style for Seven, based on blueprint mockups..
Remaining tasks
- Make the required changes to node.module, being careful of interdependencies with #1751606: Move published status checkbox next to "Save"
- Probably we'll need some minimal tests
Comment | File | Size | Author |
---|---|---|---|
#90 | 90.patch | 5.08 KB | sun |
#90 | interdiff.txt | 566 bytes | sun |
#89 | 89.patch | 5.06 KB | sun |
#89 | interdiff.txt | 3.61 KB | sun |
#88 | 1838114-89.patch | 5.25 KB | swentel |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedThanks ry5n, sounds like a good plan in here.
Comment #2
larowlanTagging
Comment #3
ry5n CreditAttribution: ry5n commentedAfter reviewing the state of #1168246: Freedom For Fieldsets! Long Live The DETAILS., it's looking good but still needs work. It would be inadvisable to rely on it, so I updated the issue summary to reflect that. Also there is some possible interdependency with #1751606: Move published status checkbox next to "Save" because both are altering Node tests and NodeFormController, sometimes the same functions.
That said, here is a first patch based on the old work in #1510532: [META] Implement the new create content page design. It works, looks OK in Stark, but Seven needs work.
Comment #4
ry5n CreditAttribution: ry5n commentedThe main issue #1510532: [META] Implement the new create content page design is tracked as a task, so this should match.
Comment #5
ry5n CreditAttribution: ry5n commentedI guess this is a feature after all.
Comment #6
ry5n CreditAttribution: ry5n commentedOK, a patch for review. This gets us most of the way. One bug I know of is that the 'create new revision' checkbox is checked by default for creating new nodes, which is wrong. I need help fixing that.
This patch also fixes the form api bug that fieldsets with #title_display set to 'invisible' were not given the proper 'element-invisible' class.
Comment #7
ry5n CreditAttribution: ry5n commentedSince this is part of #1510532: [META] Implement the new create content page design, which is major, making this match.
Comment #8
ry5n CreditAttribution: ry5n commentedFinally, a screen of what this looks like in Chrome.
Comment #8.0
ry5n CreditAttribution: ry5n commentedDecided not to rely on #1168246; noted possible interdependencies with #1751606.
Comment #8.1
ry5n CreditAttribution: ry5n commentedAdd links to the layout issue.
Comment #8.2
ry5n CreditAttribution: ry5n commentedShortlink to layout issue didn't take.
Comment #10
ry5n CreditAttribution: ry5n commentedI need help fixing these failing tests. Some of the changes in NodeFormController may be the culprit, or the addition to form.inc to allow #title_display for #type fieldset. I've been digging around but haven't made much progress. Again, I need help to get this in before feature freeze!
Comment #11
swentel CreditAttribution: swentel commentedI think this one will fix a lot of of tests. NodeFormController.php contained code for the existince of the path alias - see interdiff - but the module is not always enabled. Also, that shouldn't belong there anyway, it should belong in path.module. I assume that code is for the new content page design, because I don't those items on my screen at all - unless I miss something. Anyway, just removed it for now, let's see how many fails we have left.
Comment #12
ry5n CreditAttribution: ry5n commented@swentel Thank you! Let's see what testbot thinks…
Comment #13
sunHm. What's visible in the OP's screenshot looks 100% like details to me? At least the shot depicts more or less exactly how user agents are supposed to render details elements...?
Comment #14
ry5n CreditAttribution: ry5n commented@sun: Yes. I want to use
<details>
here, but that patch is not in yet. Current implementation uses the normal collapsible fieldsets, I am hoping that it will require minimal refactoring once the details patch is in.Comment #16
swentel CreditAttribution: swentel commented@ry5n - only 5 exceptions, but should be easy to find, it's only notices. I'm off to other patches again :)
Comment #17
ry5n CreditAttribution: ry5n commented@swentel Thanks a million. I think I know where those remaining notices are coming from, too. Let's see:
Comment #18
ry5n CreditAttribution: ry5n commentedWoohoo! I've tested in Stark and in Seven. Just needs accessibility review and code review.
Comment #19
nod_Touches a JS file.
Comment #20
ry5n CreditAttribution: ry5n commented@_nod Ah, good catch! This made me realize that this change to node.js and also one other line in NodeFormController are actually duplicate code changes from #1751606: Move published status checkbox next to "Save" and are already included there. New patch removes those lines (so no JS changes).
Comment #21
eigentor CreditAttribution: eigentor commentedAgain, just a functional test. Works as expected in all browsers I tested.
Hopefully this will attract some savvy people to do the code reviews.
The fieldsets do not have accordion functionality at the moment and all stay open. I guess that is intentional and was the last consensus, or is not part of this patch?
Comment #22
ry5n CreditAttribution: ry5n commented@eigentor Thanks! Yes, there is no accordion functionality (as in, only one open fieldset at a time); I believe this is by design.
(Just a note for reviewers: the screenshot in #21 includes changes from #1751606 as well as this patch.)
Comment #23
ry5n CreditAttribution: ry5n commentedNew patch hides the fieldset summaries with CSS
display: none
. I'm very much open to better ideas.Comment #24
joelpittetThink it would be sensible to remove the Author line from the Node->isNew.
It would be one less line and considering the node isn't saved and the user can change the author to someone else, it may save some confusion brought by the autocomplete author change not being reflected in the summary.
That OR we always keep them in sync similar to the way the summaries are updated with Jquery in node.js with
drupalSetSummary()
Something to the effect of
Comment #25
ry5n CreditAttribution: ry5n commentedNew patch merges in changes from HEAD, and fixes the following:
Looks like there are also bugs stemming from #1168246: Freedom For Fieldsets! Long Live The DETAILS.:
Comment #26
yannickooWhy you are using the
[id="edit-settings-summary"]
selector and later#edit-publish-state
?Comment #27
ry5n CreditAttribution: ry5n commented@yannickoo Selector specificity. I don’t want to add specificity unless it’s needed; using id selectors carelessly can cause all sorts of problems with related styles being unexpectedly overridden. So I used the attribute selector to traget the id.
With #edit-publish-state, that is truly a unique element with no descendants, so using the id selector shouldn't be problem and is slightly more performant.
Comment #28
eigentor CreditAttribution: eigentor commentedWorking as expected http://screencast.com/t/FL9luIsf . Code review wanted.
Comment #29
attiks CreditAttribution: attiks commented#26 & #27 same question, but probably me, what is the difference between [id="edit-settings-summary"] and #edit-settings-summary
why don't your write #edit-settings-summary
why don't your write #...
why
why don't your write #...
#28 I don't see this, am I missing something?
I tested this with a user with no access to revisions, see screenshots:
Comment #30
ry5n CreditAttribution: ry5n commented@attiks
[id="thing"]
has specificity 0,0,1,0 (same as.thing
) while#thing
has 0,1,0,0. Practically speaking, that means that#thing a
will override.thing .subthing a.my-special-link:hover
, which can get frustrating, especially if the first selector is in another stylesheet. I've gotten into the general habit of avoiding id selectors for this reason. Also, I would not normally write styles like this, but I don't yet know how to abstract this into a reusable UI component based on classes. I know it looks a bit weird. Does that help to explain?Comment #31
attiks CreditAttribution: attiks commented#30 Yes it does, I thought they were the same, thanks for explaining.
Comment #32
ry5n CreditAttribution: ry5n commented@attiks Ah, good! Also, thanks for catching that bug with user permissions and revisions. I’ll take a stab at sorting that out.
Comment #33
ry5n CreditAttribution: ry5n commentedHmmm… I’m not having the easiest time figuring out the logic for the revision fields display. Can anyone help who knows the node and revision systems? Just need to make sure the revision fields get shown/hidden for users with appropriate permissions.
Comment #34
Gábor Hojtsy@ry5n: not sure about your question on revisions. Can you elaborate a bit?
In the meantime here is a review of the PHP code :)
Minor code style issue.
It looks strange to me that a settings summary is not just a summary, it contains actual settings. I'd rename this to something appropriate like "publication_settings" or somesuch.
....
Except that it looks like it only contains the revision settings. Hm? Why?
As per Drupal coding standards, this should be FALSE.
Erm, what do you want/need this additional settings item for? Would this be a place for contrib stuff? Would be good to document.
Comment #35
ry5n CreditAttribution: ry5n commented@Gábor Thanks for the review; much appreciated.
About the revision fields: the textarea (not the checkbox) seems to display to users without revision permissions (see #29). I haven't been able to figure out why yet; the logic seems to be the same as before this patch. It's probably simple, but it could use another set of eyes.
The $additional_settings is the trickiest problem. In D7 it was the vtabs container; many modules (including node module) add their node settings to this element using the Form API #group property. However, it should not be type=details. There's a separate issue for this problem: #1856178: '#group' Form API property only works with <details> elements. For now, I'll comment this with an explanation and a @todo.
Also, I agree with changing the name of $settings-summary. I'll reroll with a better name for that container and fixes for the coding standards missteps.
Comment #36
jessebeach CreditAttribution: jessebeach commentedCode
Generally the CSS looks good. Using attribute selectors to get ids to keep the selector weight down is quite clever.
Watch for code styling issues like this. The properties should be in alphabetical order. For the most part they are. Just a few outliers:
Webkits are the only browsers that need prefixes for gradients nowadays.
I'd rather see the selector weight and cascade placement determine style overrides and not an
!important
.!important
is like adding1,0,0,0,0
to your selector.Where possible, sizing that should vary with the size of fonts should be declared in a relative unit, like
em
Decimals should be preceded by a zero.
Accessibility
If we're going to use the
details
element, we'll need to polyfill it heavily. Currently only Chrome supports it, no version of IE, and it has zero accessibility support. So as it stands, it's a little worse than adiv
right now :)http://html5doctor.com/the-details-and-summary-elements/
We might add a Modernizr test for details.
More info about accessibility of the
details
element: http://www.accessibleculture.org/articles/2012/03/screen-readers-and-det...Comment #37
sunWe should discuss that in a separate issue first. I already noticed that the new Toolbar landed without any non-webkit prefixes, and at least on my local copy of Firefox stable, I don't see any of the fanciness that apparently exists in Chrome there. In short, I wish that statement was true, but alas, I cannot confirm it through actual testing.
Comment #38
jessebeach CreditAttribution: jessebeach commentedGlad to provide some test images.
Here's a red to yellow gradient using
linear-gradient
in Firefox 17 (stable)And the same in Opera 12.11 (stable)
and in IE10
FIrefox 17.01 in Windows
caniuse.com lists Firefox as being prefix free for the stable and stable-1 releases: http://caniuse.com/#search=linear-gradient
side note: the gradient in the Toolbar is very subtle. Perhaps too so:
background-image: linear-gradient(rgba(255, 255, 255, 0.125) 20%, transparent 200%);
Comment #39
ry5n CreditAttribution: ry5n commentedPatch attached addresses Gábor’s points in #34 and Jesse’s points in #36, except with regards to vendor prefixes. I just want to double-check before I remove ones for Opera and FF. Firefox for example went unprefixed with v16, which was only two months ago. Is there an existing Drupal standard for supporting older 'modern' browsers? This may indeed be something for another issue.
About the details element, it's what Drupal now uses for collapsibles so I'm not sure there's another option for that. Work on a polyfill is happening in #1839158: Replace collapse.js with a proper polyfill for <details>. Also, this patch remains funny-looking in Chrome until #1856178: '#group' Form API property only works with <details> elements is solved. However, I think both are beyond the scope of this issue.
Thanks to all for the reviews; hopefully this is close.
Comment #40
Gábor HojtsyI think the visibility of the revisions textfield is by design - not actually a problem? Looking at the code, the fieldset has more flexible conditions for visibility vs. the checkbox and that was like that before or after the patch either way. So if you don't have administer nodes, but you are creating a new revision (such as due to forced node type defaults), you would have the possibility to provide a log message. This sounds like a feature, not a bug. Maybe worth a comment on the code so people coming after us are aware :)
The wrapper used to have this access check. So the whole fieldset either showed or not if you had administer nodes or you were creating a new revision.
However the checkbox will only ever show if you have administer nodes. So if you create a new revision but don't have admin nodes access, you see the textfield but not the checkbox.
Comment #41
jessebeach CreditAttribution: jessebeach commentedTurns out that the Modernizr build in core is checking for details
And the modules page form is using details. ry5n, did you look at this code when you were putting this patch together? Just want to make sure we using the same patterns.
Comment #42
ry5n CreditAttribution: ry5n commented@Jesse The short answer is 'yes' :)
All of core's collapsible widgets now use
<details>
. Instead of specifying'#type' => 'fieldset', '#collapsible' => TRUE
, all that’s required is'#type' => 'details'
. This was done globally in #1168246: Freedom For Fieldsets! Long Live The DETAILS. and nothing had to be changed for this patch. However, as previously noted (#39), the $additional_settings element should not be a 'details' and should be changed to a simple container once there's a fix for #1856178: '#group' Form API property only works with <details> elements.Comment #43
ry5n CreditAttribution: ry5n commentedSo where are we with this? I think we’ve addressed all concerns brought up so far… Is there anything else that needs work?
Comment #44
swentel CreditAttribution: swentel commentedNew patch with one small change regarding the default state of the revision checkbox which now defaults back to the default behavior we have in core. @ry5n - is/was there a specific reason why this was done the way you implemented it in the patch, because I couldn't find an immediate reason for it.
Next up, I'm going to look at #1856178: '#group' Form API property only works with <details> elements, because, as seen in the screenshot underneath, this is not really nice, and probably a blocker to commit.
Comment #45
swentel CreditAttribution: swentel commentedPosted a patch over at #1856178: '#group' Form API property only works with <details> elements which allows to 'group' elements in the 'container' type. If you change the #type in the 'additional_settings' to 'container', this result will look like screenshot underneath.
Comment #47
swentel CreditAttribution: swentel commented#44: core--node-form-collapsibles--1838114--44.patch queued for re-testing.
Comment #48
ry5n CreditAttribution: ry5n commented@swentel Thank you for working on this, also the #group issue! About that checkbox, I find it confusing that ‘create new revision’ would be checked before the node is even created: something ha to exist before one can revise it! Perhaps this is a UI issue? I can understand why under the hood, there wouldn’t be a difference…
Comment #49
swentel CreditAttribution: swentel commented@ry5n - I agree it's indeed a bit confusing. It's the same behavior as in core right now, but I guess it would make sense to not toggle it when creating a new article. I'll have a look at it as we can rely on the nid being set or not.
Comment #50
swentel CreditAttribution: swentel commentedAs an addition to #48 en #49 - maybe it would make even more sense to hide the checkbox and the textfield completely on node/add/*, that was how the original patch worked right ?
Comment #51
sunHm. Why is this patch still implementing fieldsets? Details are in, let's use them.
With that, the majority of the CSS should be obsolete. Likewise, the functional code changes should be minimal.
Details are "Collapsibles", by design. Based on the screenshot in #0, the primary desired change here seems to be to just simply remove the vertical tabs container?
Comment #52
sunI've rewritten the implementation and vastly simplified it.
Also adjusting issue title to clarify the actual goal.
I have a lot of concerns with the proposed design, but need to think about them some more. The current, prepended meta info container doesn't make sense, since it duplicates editable form controls elsewhere in the interface. I saw "Published" and wanted to click it to change it. I saw the author name, and wanted to change it. I didn't see the authored/creation date anywhere (and I wanted to change that, too).
Comment #53
ry5n CreditAttribution: ry5n commented@sun Can you detail the issues with the existing implementation? It did in fact use details for the collapsible components, as the details patch went in partway through this issue. Sorry for not updating the issue summary to reflect that. So, I was under the impression that #42/#44 was almost RTBC, with only #1856178: '#group' Form API property only works with <details> elements blocking.
As for the design, there are surely improvements to be made; some people may indeed be confused about the summary area at the top of the meta settings. However, this design was user-tested and results were positive. More importantly, this is an implementation issue and we should discuss improvements elsewhere.
Comment #54
ry5n CreditAttribution: ry5n commentedPutting those tags back. Gah, why do tags remove themselves like that?
Comment #55
sunSure:
- CSS should not rely on IDs, but use semantic/sensible + re-usable class names instead. The new code uses generic CSS classes, so the visual design can be re-applied to other content entities more easily. (But alas, also removed.)
- The PHP code contained too many specific customizations that were hardly overridable. Something as generic as the node form still has to remain easily customizable for varying use-cases.
- Less fieldsets/details, but containers instead.
- Simplified the markup as well as the CSS.
Speaking of, we need to make this work in Stark (and actually we should've done that first). I don't know what the battle plan is with regard to actual themes. A one-off implementation for Seven obviously doesn't fly. It's not easy to imagine how this approach can work in a generic fashion, as it is not based on CSS only, but involves changes to the actual form + markup.
Comment #57
ry5n CreditAttribution: ry5n commented@sun I agree about ids in selectors. The previous CSS in this patch is not a paragon of clean modularization or front-end best practices. However, if we’re really going to be rigorous, selectors like 'fieldset.flat' and '.entity-meta .meta .published' aren't ideal either, since they add too much specificity, risk cross-contamination, for example from other uses of both `.meta` and `.published` and are unpredictable (I can’t look at the markup and know that '.flat' will do different things depending on the element it’s applied to).
Also the visual styling in #52 is a regression, with aesthetic issues and broken styles for open/close states (open and close a number of combinations of details, you’ll see some open ones look as if they’re still closed).
@everyone I’m just frustrated. I’ve been trying my damnedest to implement this (#1510532: [META] Implement the new create content page design) for… wow more than 6 months now, including implementing it twice along the way (with help here and there from all of you, for which I am very grateful). But geez… I’m running out of steam here. I guess what I’m asking is that we wrap up this project one way or another.
Comment #58
swentel CreditAttribution: swentel commented#52: drupal8.entity-meta.52.patch queued for re-testing.
Comment #60
swentel CreditAttribution: swentel commentedI can understand the approach, attached patch fixes the tests +
Should be green.
Comment #61
swentel CreditAttribution: swentel commentedComment #62
sunWell, hrm... the suggested removal of node.js is nice, but that actually circles back into the major problem I mentioned already:
The proposed visual design is really very specific and oriented towards interpreting nodes as a content publisher facility, so it is changing the entire interface to focus on that concrete case. The two column layout (which has to be styled like this or it fails to work) reminds a lot of wiki system interfaces and also the Wordpress blogging interface.
The design is strongly centered around a administrative back-end vs. public front-end separation, typical for a few publishing scenarios.
So essentially, where this is going to fail is in all the cases in which these strong design assumptions don't hold true — when there is no backend/frontend separation, especially in terms of audience. E.g., when nodes are used as forum topics. Or as other community content. I.e., whenever you move away from the pure editorial use-case that this design is centered around.
I've built plenty of sites in which nodes were not used in an editorial context, and just simply as content (most often, community-contributed). Users of these sites aren't supposed to see publishing info, revisioning details, and whatnot - just content. This new visual design would force me to implement a completely custom entity type and whatnot in D8 to get rid of those editorial design assumptions - or at least, to alter the heck out of it in order to get back to a sane + simple node/content page.
In essence, the same story as usual - Drupal core making too many assumptions that are hard to undo. That's especially concerning here, since we're talking about one of the most fundamental, most generic, and most used facilities in Drupal: Nodes.
Please don't get me wrong, I'm not criticizing the proposed design itself. Instead, my concerns are the amount of use-case specific assumptions and the consequences of this change.
So to get back to the removal of node.js — I don't think we want to do that, because it literally disallows everyone to change the design back to vertical tabs. Depending on the specific use-case of your site, the vertical tabs can present a significantly more appropriate user experience than the editorial sidebar column, which might very well simply don't match your needs.
However, reverting the removal of node.js won't really be sufficient, since we're changing the underlying markup to no longer work in a generic way, but instead to work for a very specific, particular visual design implementation.
Which ultimately boils down to: The current built-in interface is generic, you can specialize it for your use-case. This patch makes the built-in specialized, making it impossible to go back to generic. (What's hard for some, is impossible for others.)
All in all, I really wonder whether it wouldn't be better to approach this direction within Seven theme only first; i.e., by implementing
seven_form_node_form_alter()
. — And from there, try to find ways to make it more generic.Comment #63
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps one way to think of this is that D8 core is providing a node entity which is explicitly designed for holding content. That's why it has a form that is optimized for content. Other entities provided by contrib or custom can provide own optimized form. We've special-cased node for a while and this provides good justification for doing so. We can go further and say that Seven is optimizing for the same reason. I'm fine with that too.
Comment #64
ry5n CreditAttribution: ry5n commentedYay, green patch! Apologies all around for being cranky in #57. I will pitch in to get the styles for details states sorted out.
#62 seems totally sensible, as does #63. I don't know enough about the relevant issues, so I'll go along with those who can advise.
Comment #65
Bojhan CreditAttribution: Bojhan commented@sun Is there a way we can make it more flexible, so that use cases where the "node form" is not used primarily as content delivery tool. You can easily adjust it? I feel going back to Vertical tabs for those usecases, should almost be like flipping a switch in your theme template, or perhaps even content type settings (although I'd favor, no UI settings in Drupal at all).
It seems like your concerns are mostly around making this a flexible implementation, so our assumptions which are probably accurate for ordinary content creation do not interfere when you are doing non-ordinary content creation for other purposes; e.g. forums, products, media - where indeed a sidebar might not make much sense.
Comment #66
swentel CreditAttribution: swentel commentedMakes sense to me, I'll patch that up next week, unless someone beats me to it this weekend.
Comment #67
ry5n CreditAttribution: ry5n commentedFixed CSS for the details states and made the CSS hooks for the meta header/summary area a little more robust.
Comment #68
ry5n CreditAttribution: ry5n commentedCan we use the layout system to do the column layout if this is theme-dependent? The next step after this patch is #1838156: Implement the two-column layout for the new create content page, which right now is halfway implemented but is waiting on this to go in.
Comment #69
swentel CreditAttribution: swentel commentedThat's going to be a tricky, nearly a no go as the layout system has no conditions support atm - and won't have very soon too I'm afraid. I'm on that issue next week as well and investigate some more.
Comment #70
sunI'd agree that "swappable and pluggable form layouts" would be the ideal implementation. But as @swentel mentioned, layouts/displays are still in their early stages + parallel work on fields/fieldgroups/form-builder isn't there yet. Thus, I wouldn't rely and wait on that.
After hacking on this patch myself I understand it much better, and would summarize the problem space like this:
hook_form_alter()
implementations.Whether the editorial interface is suitable depends business logic, which can be assumed for some built-in content, but is unknown for custom content; i.e.:
I think we should do the following:
#type 'entity_meta_sidebar'
[insert name bikeshed here]. Essentially, a formal UI element #type that shares semantics with#type 'vertical_tabs'
. The two #types can be swapped with each other.#type
of the wrapping container/column under the hood:The value of this selection option is the actual, internal #type name; i.e., either 'vertical_tabs' or 'entity_meta_sidebar'. Contrib can introduce new container types, and even swap out the existing.
We could even consider to add a third option of 'container' here, which apparently would render just the details elements with no vertical tabs and sidebar; i.e., essentially the D6 editing interface of a simple list of collapsible details (formerly fieldsets).
This plan of attack would resolve the problem of making too many assumptions, while keeping the implementation simple.
It would also not special-case this entire nice editorial sidebar interface for Seven theme only, which is important IMHO, because I think we want to introduce this interface concept as an official pattern that "ought" to be implemented by contributed and custom themes, in order to improve the editing interface user experience for editorial content.
As a result, this will also work in Stark, and in turn, that means it is inherently tested by more people, which means that chances of mistakenly introducing regressions for the editorial interface by another patch/change are significantly reduced.
Comment #71
yoroy CreditAttribution: yoroy commentedGood to see this discussed in depth. But I would like to see one thing clarified then:
In what way is the current (with vertical tabs at the bottom) UI *a better* default for non-editorial (as you call it) content? I want to understand why having vertical tabs at the bottom of a node form means making less assumptions and thus makes it more applicable to other types of content.
UI-wise I don't see how having vertical tabs at the bottom implies making less assumptions. Is it the amount or structure of the output?
I ask because I think we have to carefully weigh how much harder it will be to undo/override the proposed new UI compared to the current situation before we add yet another setting somehwere in the admin.
Comment #72
yoroy CreditAttribution: yoroy commentedWe (Kevin, Roy, Bojhan, dcmistry & a bit of webchick) discussed this during UX hours in IRC. Consensus is still that we all think the sidebar is the better default for node content.
Can we defer the discussion to wether the sidebar should be the default for *any* entity creation form in a different issue and continue as planned in here?
Comment #73
tstoecklerSo that would mean we do #70 1., 2. and perhaps 4., but not 3. (and perhaps discuss 3. in a follow-up). That would mean the sidebar as depicted here would show for all content types, but it would be really easy to swap that out from a module (but not from the UI! (at least not for now)). @sun, would you be on-board with that?
Comment #74
swentel CreditAttribution: swentel commentedI would try and keep focus on the original issue and discuss 2 and 3 in the two column issue, we have less comments there, so we can bikeshed like hell ;)
Updated patch:
This is a different approach and makes it 'administration (theme)' dependant, not '#type'. Basically : details (and then soon 2 column) in admin, vertical_tabs in 'frontend'. This should solve most objections that sun had imo.
Adds isNewRevision() to Node.php as there was a logic error re: revisions - try logging in as 'normal' user and creating a forum topic.Comment #76
ry5n CreditAttribution: ry5n commented@swentel Looking good! Happy to see the #group pach is now RTBC and working well here.
Comment #77
swentel CreditAttribution: swentel commentedReroll after #1856178: '#group' Form API property only works with <details> elements is in, so should be green. Slightly optimized the logic for hiding the 'meta' block when we're not in administration mode.
Comment #79
swentel CreditAttribution: swentel commentedBah, left out the fix re: isNewRevision() to Node.php, we can always tackle that afterwards.
Comment #80
moshe weitzman CreditAttribution: moshe weitzman commentedCode looks good. The node form actually looks a bit uglier with this patch as we really need #1838156: Implement the two-column layout for the new create content page to fix it all up. swentel plans to work on that tomorrow.
Comment #81
Dries CreditAttribution: Dries commentedGreat intermediate step! Committed to 8.x. Hopefully the two column layout follows shortly after as that will make this look pretty again. :)
Thanks swentel, ry5n and sun!
Comment #82
tstoecklerCan we get a quick follow-up to remove the double #type? That just melted my brain... :-) Thx!
Comment #83
swentel CreditAttribution: swentel commented@tstoeckler ok if I just pick that up in #1838156: Implement the two-column layout for the new create content page ?
Comment #84
tstoecklerSure. Don't want to hold anything up. :-)
Comment #85
ry5n CreditAttribution: ry5n commentedYay! Just want to say thanks to everyone who helped with this. So happy to see this in :)
Comment #87
sunThis part of the committed code does not work like it should:
You're not able to trigger the non-vertical-tabs presentation, unless you have the obscure checkbox in Appearance settings enabled that says:
"Use the administration theme when editing or creating content"
In short:
1) Install Minimal.
2) Enable Seven theme.
3) Go to a node/add or node/edit page.
People are reporting bugs against other patches, because they are getting an entirely different user interface.
Comment #88
swentel CreditAttribution: swentel commentedHow about this: use seven_form_alter(). I was already thinking todo that anyway in the two column patch. This removes lines from NodeFormController::form that I already felt a bit annoyed by anyway.
Comment #89
sunWorks for me. As long as we don't have form displays in core, that's at least a little bit more predictable.
Merely cleaned up some minor things.
And replaced the @todo with a separate issue: #1906208: Independent .status HTML class triggers message/icon styling
Comment #90
sunAnd before anyone complains... ;)
Comment #91
swentel CreditAttribution: swentel commentedWorks perfect, thanks!
Comment #92
webchickCommitted and pushed to 8.x. Thanks!
Comment #94
steveoliver CreditAttribution: steveoliver commentedFrom #79:
I like that this "temporary hack" allows for adding attributes to form element containers without the need for extra wrapper elements ala #prefix/#suffix and without needing to override theme_form_element() just to customize the attributes of form element containers. So +1 for leaving this temporary hack in.
Any reason why we shouldn't keep this #wrapper_attributes support in place (and remove the @todo)?
Comment #94.0
steveoliver CreditAttribution: steveoliver commentedUpdated issue summary.
Comment #95
tkoleary CreditAttribution: tkoleary commentedReopened so that members who commented can review related UX regression #2372163
Comment #96
swentel CreditAttribution: swentel commentedDon't reopen - comment is fine :)
Comment #97
YesCT CreditAttribution: YesCT commentedlooks like work to add back the summaries is going to happen in #2493957: Add back errors support to native HTML5 details tag