Problem/Motivation
The current behavior of edit-in-place forces a user to make single-field edits. Each field update results in an incremented revision on the field's entity.
By allowing changes to fields to be stored temporary, we let an author make multiple field edits that can be committed with a single save action. In this way, in-place editing will no longer create a flood of entity revisions that correspond to just single field edits.
Proposed resolution
We will move the save and cancel operations to a toolbar associated with the entity, not a field of the entity. A user will be able to put an entity in to edit mode, select a field of that entity to edit, make changes and move to another field. The changes to a field will be stored on the server side in a tempstore entry.
Pressing the save button in the entity toolbar will commit all of the changes from the start of the entity in-place editing session. Pressing the cancel button will return the original values to all the fields of an entity that had been editing in place and no changes will be maintained.
Tasks
Finished
- Entity toolbar introduced with the EntityToolbarView and EntityView classes.
- Field-related toolbars (such as a WYSIWYG toolbar) are now rendered with FieldToolbarView inside the EntityToolbarView element.
- Users can move from an active, in-edit field to a candidate field. Changes to the previously active field are pushed to tempstore invisibly when switching fields.
- edit.css is broken out into: edit.module.css, edit.theme.css, edit.icons.css
- The JavaScript has been combed through for needed commenting and JSHinted.
- Front end test for editing and saving the body field of an article: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
- Front end test for editing and saving the tags field of an article: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
- Styling of the entity toolbar components e.g. the control buttons do not match core styling yet.
Remaining
- Testing of control flows.
Manual testing scenarios
- PASSED | Enable in-place editing / activate a textarea field / change the content of the field / press the entity toolbar save button
- PASSED | Enable in-place editing / activate a taxonomy field / change the content of the field / press the entity toolbar save button
- PASSED| Enable in-place editing / activate an image field / change the content of the field / press the entity toolbar save button
- PASSED| Enable in-place editing / activate a text field / change the content of the field / press the entity toolbar save button
- PASSED | Enable in-place editing / activate a taxonomy field / change the content of the field / press the entity toolbar save button
- PASSED | Enable in-place editing / activate a field / press the entity toolbar cancel button
- PASSED | Enable in-place editing / activate a field / change the content of the field / press the entity toolbar cancel button / press the save button in the confirmation dialog
- PASSED | Enable in-place editing / activate a field / change the content of the field / press the entity toolbar cancel button / press the cancel button in the confirmation dialog
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar save button
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar save button
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar cancel button / Press the confirmation save button
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar cancel button / Press the confirmation cancel button
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar cancel button / Press the confirmation save button
- PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar cancel button / Press the confirmation cancel button
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / press the entity toolbar save button / The field should me marked as invalid
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar save button / Save should fail and the first field should me marked as invalid
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar cancel button / Press the confirmation save button / Save should fail and the first field should me marked as invalid
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar cancel button / Press the confirmation cancel button / All fields should revert to their original values
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / change the content in that field / press the entity toolbar cancel button / Press the confirmation save button / Save should fail and the first field should me marked as invalid
- PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / change the content in that field / press the entity toolbar cancel button / Press the confirmation cancel button / All fields should revert to their original values
User interface changes
This patch introduces a floating toolbar that is anchored on the entity and its fields.
Original report by webchick
This came up in demo call today. This would allow for much faster data entry.
Related issues in the Spark issue queue (still to be consolidated into this issue):
- #1586416: [Meta] Make Save/Publish distinction clearer
- #1702250: [META] As a content editor, I want to make my edits quickly and then hit save
- #1780984: Test prototype for Save/Publish in one place + revision selection
- #1494640: Add a workflow/revisioning module to Spark
Comments
Comment #1
webchickComment #2
webchickAnother option was to get rid of save buttons altogether, save a "draft" revision upon leaving each field, and then have a "Publish" button to commit all "local" changes.
Comment #3
Wim LeersThe latter makes most sense, I think.
Comment #4
Wim LeersRelated: #1678034: Auto-save revisions on edit + generate log message.
Comment #5
Wim LeersRelated issue in the Spark issue queue: #1586416: [Meta] Make Save/Publish distinction clearer.
Comment #6
tkoleary CreditAttribution: tkoleary commentedHere are the current designs showing how it works with navbar. In the context of D7 if navbar does not exist in the site I'm not sure what happens. Wim, can you comment?
Comment #7
Wim LeersAs of last week, the Edit module will automatically integrate with either D7 core's toolbar module or our navbar module, whichever is available! :) (Thanks to Gábor.)
Comment #8
Wim LeersGábor tackled that in #1688166: [META] As a Drupal community member, I would like to see Edit module de-coupled from any particular toolbar implementation, FWIW.
Comment #9
klonos...and I've filed #1765558: Support edit.module (Spark) so people can use WYSIWYG editing in D7 with admin_menu. for admin_menu too ;)
Comment #10
Gábor HojtsyHere is a first patch for this. I think I'm very much "butchering it", or in other words, I feel like I'm an elephant in a porcelain shop pulling the pieces I need with little respect for the whole architecture. In retrospect I don't think I'm the right person for this, even though it seemed like it would be easy enough to do, it certainly requires lot more JS experience to do it well, of which I have least on the Spark team.
Anyway, the attached patch implements:
- addition of a gray/blue button titled "Save" on the toolbar's shortcut bar area
- click-action assignment of this to save the current editable as appropriate (I suspect this click event assignment might happen in a chain but I could not reproduce it actually ending up with multiple revisions saved, so not entirely sure)
- hiding of the per-editable save button (since some parts of the code referenced this and I did not want to remove those just yet)
- the global save button starts off as gray and becomes blue when you make changes according to the same logic as before
- remove the confirmation step when you click out of things and just doing the action instead of asking the user
I think the changes should be definitely enough to user test the new behavior but the code changes definitely need some more cleanup.
Comment #11
Wim Leers1) The functionality works well.
2) The save button is not very findable right now. I think it should be more visually prominent at all times?
3) The code indeed needs to be cleaned up, but that's probably at least partially also because all of Edit's JS should be refactored: features have been removed that have some left-behind cruft, and that will make this easier already.
I think it's a good first stab/step.
However, AFAIK the goal was to also remove the cancel button, and to have field changes save automatically (without the need to click this "Save" button), with each time a new *draft* revision being created. The cancel button would go away. We'd have a list of revisions and a way to revert to any of those revisions, as a replacement for the ability to cancel. And instead of "save" (which immediately publishes), we'd have a "Publish" button, to publish the draft revision the user is currently looking at.
All that being said, if this was just a first step to get the Save button into the toolbar, I think that's fine :)
We might want to hold off committing this, not because of code quality, but to not divert Edit's codebase from the Create.js-based fork of it on Github (https://github.com/wimleers/edit-createjs), until a decision is made on that front. If you prefer to commit this, that's fine too though :) But AFAICT, that doesn't "fix" this issue.
Comment #12
Gábor HojtsyWell, the explanation at http://drupal.org/node/1702250#comment-6356512 did not really have details as to whether the cancel button should also be removed (and indeed currently there is no better way to cancel out of a field without the revisions dropdown implemented). As for the difference between Save and Publish that is only possible to distinguish if Edit module DOES require one of the workflow modules. If none of the workflow modules are installed (or the one specifically picked to be required), then it is not possible to have unpublished revisions with a published revision being active. So implementation for that would need to integrate with a workflow module like that.
If we want to solve the whole thing here, then I'm not sure what is the difference between this issue and #1702250: [META] As a content editor, I want to make my edits quickly and then hit save. I thought this would be a somewhat limited issue to some first steps and then we can open new ones for workflow integration, etc. where we can introduce publish/save button differentiation (and rollback/cancel).
Comment #13
Wim LeersYou'e right, in my comment I was expanding this issue to the whole problem space. Sorry.
It's a very tricky thing to solve, and I agree, this issue is intended to be somewhat limited. But again, I do think it's a good first step!
Comment #14
tkoleary CreditAttribution: tkoleary commented@wim @Gabor Preston is now working on updating the prototype of this (per our consensus from Acquia engineering week). Should be ready to test by 10/23.
Comment #15
frega CreditAttribution: frega commentedI just want to make sure I fully understand this: the edit.module-UI will switch to a "Central Save"-button rather than providing per-field buttons.
This actually happens to be closer to how createjs handles it and will probably simplify the required (glue) code considerably.
I haven't looked particularly closely at the "Drupal-end of things", but how are we going to handle persisting multiple "fields" at once? Is there a more elegant way (e.g. provided by fape.module) than keeping the various loaded forms "around" and submit them in a sequence on "save"?
Comment #16
Wim LeersNope. And that's the tricky part. As I've said before: we need to be able to have instant previews of the modified fields. So even if we don't "really save" a changed field, we really do want to show the user immediately what that'd look like. It seems the only way to achieve that in a reliable manner is to leverage TempStore, which will hopefully also power full node previews. If the full node preview based on TempStore problem is solved, then we should be able to piggyback on that work as well.
I don't think we should try to solve this before feature freeze.
Comment #17
Wim Leers#1678034: Auto-save revisions on edit + generate log message merged with this issue, plus revamped the issue summary. This is now the sole issue for this topic in the Edit issue queue, the relevant issues in the Spark issue queue are linked to.
Comment #17.0
Wim LeersUpdated issue summary.
Comment #18
Wim LeersAlso merged #1741692: Discard via ESC button with this issue; issue summary updated.
Comment #19
Bojhan CreditAttribution: Bojhan commentedCopied from #1780984: Test prototype for Save/Publish in one place + revision selection another duplicate.
#19.1
On November 19 (Monday), I conducted a usability study on the new iteration of edit-in-place for Spark that focused on:
The study was to be conducted with 5 content creators/maintainers. However, after 3 sessions, the issues were clear and the researcher is confident that more sessions would not yield any new insights.
Executive Summary:
Participants easily understood how to edit their content (title, body). The effectiveness rating for the task was the highest (5 out of 5). It is also worth noting that the task rating has gone up to 5 from 4.2 (previous design).
However, saving/ publishing content (rating of 2 out of 5) and revision history (rating of 2.1 out of 5) still needs work.
Major issues:
Minor issue:
Detailed Findings
1. Does “Revert” auto-publish my content?
Refer Skitch Image attached in the next comment.
Participants were thoroughly confused if revert was changing their published content in real time and hence gave poor ratings to the task (2 out of 5, 5 being best). This is because of the following reasons:
Requests to revision control:
2. Since this article is already published, is hitting “Save” auto-publishing my content?
Participants thought that saving fields is saving the page and since it is already a published page, it is also pushing the “saved” changes LIVE.
Reasons for this problem:
3. Up/down arrow on revision control not useful [Minor Issue]
2 of the 3 participants did not immediately understand what’s the purpose of up/down arrow on the revision control. Once they did understand, they thought it was “useless” as it is easier to click on a particular revision from an exposed drop box than to go to one revision up or down.
Other issues:
Observations
#19.2
This is definitely worrying, I have seen earlier iterations and it seems like this is a tough nut to crack. The overall trend seems to be that it is hard to communicate to users, that your "front end" isn't necessarily what your visitors see but could just be a new revision. That whole concept, seems hard to communicate - and rightfully so, we generally keep the front-end reserved for the end result unless you make an explicit step to "demo" something.
I guess the whole idea of "please don't forget to press save" what we have on blocks, now just turns into "please don't forget to press publish". Partly because its not really clear, what the version is you are looking at "Current" (as of now does not signal that) and that you need to "Publish" to get it to show to visitors, "Save as draft" will do something to help there but there conceptually will still be a disconnect - there is no large "you are not looking at the live thing". The fact that you uncovered even more issues with reverting, to me only sounds natural that is quite tricky functionality. Concrete 5 for example, has a really flow around this all where there is a clear "preview my edits" and "publish my edits", where preview allows you to continue, publish just does what you mean and there are additional links and forms for revisions, deleting etc.
We use "advanced edit" as a label for the full editing interface, I am not sure about it could scare people of from the only interface you can add it to a menu, add revisions info etc. "Quick edit" is an established label, for inline editing I would love if we could use that.
In general I am wondering if this might be trying to tackle something, that simply doesn't fit this paradigm. Although I am sure we can come up with something that, makes it a little more clear. I am not sure if this inline editing fits audiences where its important to care about publishing workflows - e.g. how does this scale when you have many revisions, authors reviewing, several states the content can be in, and diffing (you would replicate a whole screen in one tiny area). Its probably worthwhile to ask those in such settings, whether they'd deploy this beyond sexiness.
Wouldn't we be much better of, just supporting inline editing and pushing straight to published? And allowing contrib to come up with a model to support more advanced workflows. I guess this is not a popular question, but I am having difficulty following who we are designing this particular part for and it could greatly simplify it for the other usecase.
Comment #20
lightsurge CreditAttribution: lightsurge commentedI like the idea of in-place editing always putting alterations live. So 'advanced edit' might become 'revise' and 'edit' (i.e. IPE) becomes adjust/tweak/amend or whatever. So you have separate permissions for those who are allowed to amend, which allows one to make immediate published tweaks, as well as revise - revisions which might or might not be subject to a moderation workflow.
As far as 'save as draft' goes a 'draft' watermark might be helpful, I think a lot of people are accustomed to seeing those and it might help with the UX:
http://www.memo-template.com/memo-with-draft-watermark/
Comment #21
lightsurge CreditAttribution: lightsurge commentedSo it becomes a bit like the PDF workflow, you build your document in word (i.e. bit like the Drupal nodeform), once that's published and becomes a pdf, there's certain amendments and tweaks you can still make inline on the published pdf. If you want to do anything too spectacular though, you have to open up the original word document once more and make your changes, then re-publish it.
That's a process many people might be familiar with if you can make the workflow feel like that.
Comment #22
Bojhan CreditAttribution: Bojhan commentedPer https://drupal.org/node/1824500#comment-6802542 , I am opening this. Lets stop with the silly postponed issues, its clear people want to discuss this.
Comment #23
webchickFine, but reducing it from critical, because it shouldn't block initial commit.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedFor what it's worth, the thing I brought up in #1824500-59: In-place editing for Fields doesn't seem related to the original purpose of this issue, just comments #19 onward.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedAs mentioned in #16, Tempstore is a great way forward for holding work in progress. Then a user clicks on a button the toolbar to permanently store the changes in progress. To me, this is much preferable to storing many revisions while editing the fields in an entity.
Comment #26
Wim Leers#25: We'd first need to investigate whether that's feasible at all; we still must be able to re-render changes "saved temporarily" to the TempStore for the end user.
(Or: pretty much what I said in #16.)
Comment #27
klonosYeah, I too really don't like the idea of multiple revisions for various fields edits. If it can be done with Tempstore and then in one go by hitting the/a "Save" button, then that would be ideal.
Comment #28
tstoecklerSeems it should be possible to configure that, no?
A little checkbox that asks whether you want edit.module to create a revision each time you save something and if you enable it, a (token-enabled) text field appears where you can enter the automated revision log message.
I think we can't get away with hard-coding either option; i.e. there are use-cases where revisions are completely unneccessary and only clutter the DB and the UI, and use-cases where they are integral to the workflow.
Comment #29
Wim Leers@David_Rothstein: while it definitely looks like revision handling is not intended to be part of the scope of this issue, I can assure you it is. E.g. see the design in #6, at the top it has a "Last saved" dropdown, which are revisions, listed by the time they were saved. Also see the related issue in the issue summary, which were and are still to be consolidated.
Comment #30
klonos... #1860402: Introduce a tempstore layer to in-place editing, similar to Views, that allows for atomic revisions of multiple page edits
Comment #31
tkoleary CreditAttribution: tkoleary commentedPut this issue on hold since revisions to the designs may render it unnecessary.
Comment #32
Bojhan CreditAttribution: Bojhan commentedLet's make sure others can discuss, if you put out a design that should fit right into the discussion. No reason, to stop others from discussing.
Comment #33
Bojhan CreditAttribution: Bojhan commentedMoving this to Drupal Core, also changing the status (although probably someone will revert it anyway). It is a critical bug, because currently essentially you cannot make changes using Edit without directly publishing them -this is a feature regression. Since it is enabled by default, we will expect a large part of our audience will use this - not supporting any kind of workflow, is quite a problem.
Comment #34
Wim LeersHow can this be a regression if it's new functionality?
I agree that it's critical, but it's not a bug. Rest assured, we will address this.
Comment #35
tkoleary CreditAttribution: tkoleary commentedSee this related issue on save and publish http://drupal.org/node/1898946
Comment #36
Bojhan CreditAttribution: Bojhan commentedYhea, honestly - for some reason that was opened as a new issue and its pretty much all about what we intended to fix here. I am open to closing this all and just adjusting the priority of the other one.
Comment #37
webchickWe just had an epic phone call with Dries on this issue and here was the outcome (I think; others on the Spark team, feel free to correct me if this is wrong):
- Saving should be moved per-entity, rather than per-field... UX tests already back that up and it didn't seem like there's much disagreement on that point.
- This means we'll need to save incremental edits between fields to a "tempstore" type of thing.
- Only upon clicking the "Save" button on a floating "entity" toolbar will actually commit the changes to the database. What happens here will depend on how the content type is configured: publish by default, create a new revision, and/or save directly over the existing revision. Edit module just does the save, and doesn't need to worry about it.
- There was also general agreement that Edit module shouldn't become a workflow management module... by itself, it should not be providing the capability to save things as draft or what have you. It's simply a "What you see is what you edit" kind of module. If you're viewing an published node, it will edit the published node. If you're viewing an unpublished draft, it will edit the unpublished draft.
- What it should do though, is warn you if there are unpublished revisions ahead of yours and invite you to visit the full edit form what that revision loaded. This is also the workflow if you want to save as draft. Don't use quick edit. Use the node edit form. Quick edit can make subsequent changes, however.
Kevin's been actively working on designs for this, should have something to show next week.
Comment #38
tkoleary CreditAttribution: tkoleary commentedThere have been several design iterations some testing, an invision prototype and a good deal of discussion in the usability group on this since my last post but I *think* we are coming to a consensus (at least in Spark and the usability group) on the current approach, described in this prototype: http://www.acquians.info/Public/Edit/spark/blocks/index.html
A few things to note:
Features that are not yet in the prototype (but should be soon)
Comment #39
Bojhan CreditAttribution: Bojhan commentedWe have been following this, in the group and IRC - thanks for keeping is so involved! I discussed this with yoroy, it seems like we arrived at the right concessions to create a easy to use interface. It's also very useful to have the frame of mind as you noted it down. We believe that we can push forward with this :) - I hope we can quickly get some data to see if we are in the right direction.
We shouldn't try to solve everything, I think a lot of features you note are also possible in followups or an actual patch if its too much work to prototype - with the time frame for release, we should keep in mind that this is one of many important UX issues that still need to be resolved.
I worry about the styling, because its not really in sync with what we have been doing with the style guide (round corners, connected elements, typography, color use age) - however that is also something that can be dealt with separately. The same goes for the new contextual link styling, it feels like that could be a separate issue.
Comment #40
Wim LeersYes — IMVHO — we should focus on functionality now and worry about consistent styling/presentation later.
Edit has gone through so many evolutions that IMVHO we should first get all functionality right first, spend as little time as possible on styling while reaching that "feature complete" phase, and only then work on finalizing/improving the styling & animations after that.
I'm glad the response is so positive :) Hopefully we can now move forward very fast on this.
Comment #41
tkoleary CreditAttribution: tkoleary commentedBojhan:
This relates to another issue in that admin tools that appear on the front end need a systematic way to inherit the admin theme styles. Jesse solved this for toolbar but I'm not sure if that extends to other modules or not. I will ask today. If we can accomplish that then the module's theme css can be overridden by 7 (or any admin theme).
For the moment I will adjust the radius and colors to match the new 7 style guide.
Comment #42
Wim LeersGreat point Kevin!
That's right: the style guide Bojhan refers to is for Seven, i.e. the back-end/admin theme. But this is on the front-end, not the back-end! It's impossible to automatically inherit Seven styles, so it's at the very least very questionable whether we should enforce Seven-like styles in Edit. It would look weird when you were using a different (non-Seven) theme…
Comment #43
Wim Leers#1898946: [Meta] Simplify and reconcile the behaviors of edit-submit actions in node/edit and edit (edit-in-place) has been closed as a duplicate in favor of this one because of the direction explained in #38 and supported in #39 by the Usability folks. It does contain useful background information though (and helps understand why the direction in #38 has received so much support).
Comment #44
Wim LeersThis is the front-end part of the entity-level toolbar for Edit as proposed in #38, and the +1 from the usability team at #39.
The back-end part (to be able to create one revision for all changes, and have the field edits "queue up") is going to be tackled at #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits, see #8 specifically.
Both issues should be able to progress pretty far independently, in parallel.
Gábor will be working on the other issue, Jesse will be working on this one. I will be working on making Edit work with other entity types than nodes, with the "extra fields" (node title/author/date) that aren't proper Field API fields yet, and on making Edit work with Views.
Comment #45
dcmistry CreditAttribution: dcmistry commentedI recently conducted a moderated usability study on the latest Spark prototype. The focus of the study was:
Participant Information
Summary of findings
The prototype received an overwhelmingly positive response to the ease of use and effectiveness of inline editing and created a delightful experience for its participants and got an average effective rating of 4.8/5 (this is perhaps the first time I am using “delightful” in my Drupal reports). I would encourage you to read the participant quotes at the bottom of this comment.
Positives - Participants Understood
1. How to find “Edit” and pencil icons to get into the task (4 out of 4 participants). The use of universal symbols was appreciated.
2. How to edit a field (4 out of 4 participants)
3. That X is going to discard their changes (3 out of 4 participants)
4. How to get to full edit page (3 out of 4 participants)
Issues
High Issue: Participants who are new to Drupal/Gardens (3 out of 4 participants) either thought that save is going to generate a draft/not publish or weren’t sure what’s going to happen. This issue is exaggerated, as the UI doesn’t provide user feedback after the changes are saved.
“It has to be blatantly obvious that it is going to publish or not.” (P2)
Ideally, the “Save” on inline editing should mimic the “Save/Publish” interaction on the full node edit page. To re-iterate from previous studies, “Save” does not imply “Publish” to especially new/beginner Drupal users.
Medium Issue: Participants like that the UI provides the number of unsaved changes but they thought it shorts fall in terms of functionality since it doesn’t highlight the changes. (I understand there are technical limitations to do this but I think it is good for us to be aware of the issue.)
Minor Issue: The orange/blue outline to the fields that are changed/unchanged is too subtle and doesn’t receive user attention. Participants weren’t completely sure why they were in different colors. I would say this is a minor issue since this is aimed to be a more gradual over the period of time learning functionality.
Participant Quotes
1. “This is a huge advancement in comparison to Wordpress and Joomla.” (P3)
2. “I have never used anything like this before.” (P4)
3. “I have never done this before and was able to do it easily ...it’s pretty evident the people behind this project knew all the options and made it easily available to me.” (P4)
4. “I can jump into this and create a website within a week. It looks seamless … UI looks pretty good” (P1)
5. “It looks painless” (P1)
6. “This will help the marketing team... inline editing will drastically improve performance” (P1)
7. “This will have a smaller learning curve compared to Drupal Gardens” (P1)
8. “It is user friendly to someone who is not a developer” (P2)
9. “Very easy to go and make quick edits” (P2)
Other Documents
Spark Moderator’s Guide
Recording# 1
Recording# 2
Recording# 3
Recording# 4
Comment #46
webchickWow, I'm really excited about the positive emotional reactions from people. :D Awesome news!!
That save/publish distinction is indeed a biggie, which has come up in other tests, so we definitely do need to solve that. Kevin suggested a small tweak to the button wording of "Save and publish," which I think works. He's also playing around a bit with the visual affordances on the fields to make them a bit clearer.
It sounds though like this general direction is going to work UX-wise, so we should be able to start on implementation in the next day or two.
Comment #47
webchickOMG AND RECORDINGS, TOO!! THANK YOU!!!! :D
Comment #48
dcmistry CreditAttribution: dcmistry commentedI am excited too! :)
Regarding save/publish - I had a chat with Kevin too and I also thought that his approach works.
Comment #49
ry5n CreditAttribution: ry5n commentedThis is great news. Congrats to the Spark team and thanks @dcmistry for doing this testing!
Comment #50
Bojhan CreditAttribution: Bojhan commentedThanks for sharing the results, its great to see testing so quickly. This does seem to confirm the direction, so lets move forward on the more detailed stuff.
I think the High issue, mirrors a problem that we kept finding on the content creation page. The "Save & publish" solved this problem. I think a issue, is other workflows - which we are not handeling here. I imagine however if the node is unpublished, it should just say "Save" ?
The medium issue. I really love this detail in the design, I agree that we would like to offer more information on which fields arn't saved and if possible using revision to show what changed. I guess, that is going to be real tricky - at the very least we can do a indication that shows which fields are not saved. Like a fade in?
The minor issue. Having watched the videos, I am not quite sure if using just color is a good indicator for this. Keep in mind for a11y reasons, we can't have just color as an indicator anyway. Ideally we have an indicator that we can use throughout core to indicate a change, has not been saved yet - for this we currently use a yellow background with a orange astrix. As Dharmesh mentions though, this pattern too requires some learning. I am not sure if for a feature like this, which is very front-facing that is a good end solution. But this is definitely not the only place we battle with this problem.
I am keen on seeing this implemented further, I imagine there are a bunch of issues that will come up. For example, needing to shorten node titles in entity toolbar because they might become super long (especially hurtful on mobile), integration with our existing pattern for contextual links (I am not quite sure we should break that), unpublished content, several content items on one page, etc.
Anyways, quite excited too :)
Comment #51
cbrookins CreditAttribution: cbrookins commentedre: "I imagine however if the node is unpublished, it should just say "Save""
I would think "Save Draft" would be clearer as to what it was saving (since 'Save' alone could mean publish too). Also not sure how Publish doesn't imply Saving, so why not "Publish" instead of "Save and Publish"
So recommending the two states be "Save Draft" and "Publish"
Comment #52
eigentor CreditAttribution: eigentor commentedGreat test, even greater to have the videos to draw conclusions ourselves.
Good choice you resorted to the pencil icon. Across all Content Management systems, web applications etc. it appears to have gotten some kind of standard. So having a pencil icon on something should give a lot of people the notion they can edit this particular piece of content.
I also liked the Save button becoming blue when there is something to save.
I wish the switch to edit mode on a particular field or entity would be that smooth in core :P
It is encouraging the first participant (only watched one) found the edit icon in the toolbar. I was always worrying the toolbar was too cluttered and people might not find that most important button.
The Publish / Save thing is not so easy. Maybe the Dropbutton is the best choice. There has been a lot of back and forth concerning if two buttons rather confuse than inform (I remember finding it confusing and scary on Wordpress when they had that big prominent "Publish" button.) With the dropbutton, you have "Save and publish" or some other words that turn out to be least confusing in the end. As the dropbutton is a UI element used throughout Drupal in a fairly consistent manner people will eventually get at ease with it (So I hope :) ).
Comment #53
dcmistry CreditAttribution: dcmistry commentedGlad to hear that you found the study and the videos helpful.
I am more inclined to "Save Draft" and "Save and Publish". That being said, I am not opposed to "Save Draft" and "Publish" as it conforms to other CMS workflows and users do understand that "Publish" will save their content.
Comment #54
tkoleary CreditAttribution: tkoleary commented@cbrookins
There's no ability to save a draft from edit in place so the only option is "Save & publish"
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedThere has to be that ability actually (not as a separate option, but as one of the possible behaviors on clicking "Save", depending on the current state of the content). Like it says in #37:
So it won't ever be able to take a published node and turn it into a draft, but I'm not sure that matters for the button label. The user probably still needs some kind of visual reinforcement since they won't know what rules the module is following behind the scenes.
Comment #56
Wim Leers#55: correct. I'm fairly certain that's what Kevin (@tkoleary) meant :) It's just going to leave the node in the same state.
Comment #57
dcmistry CreditAttribution: dcmistry commentedAgree to #55. Perhaps, "Save and Publish" or "Publish" (or whatever we plan to call it) could be the primary action and the "Save as Draft" be the secondary action.
Comment #58
Wim Leers#57: No! David Rothstein is saying that if you in-place edit a published node, that "Save" effectively means "Save and publish", and that if you in-place edit a non-published node, that "Save" effectively means "Save as draft". So: leave the node in the same "published or not" state.
It should *not* be possible to modify the "published or not" state from in-place editing. That's what "Full edit" is there for.
Comment #59
lightsurge CreditAttribution: lightsurge commentedI think 'Save', or 'Save changes' is sufficient, people might have all sorts of weird workflows ('second draft', 'pre-publish' and on and on). If people want to give more clarity to what workflow state the content is in, doesn't seem unfair to expect them to hook and tuck and accomplish that themselves.
'Save changes' works well for me, i.e. I've just made some changes on existing content, do I want to save those. But Save would do ;-)
Comment #60
Wim Leers+1 for #59.
Comment #61
lightsurge CreditAttribution: lightsurge commented#55
When it comes to IPE, I can see lots of what seem to me to be quite nice ways to communicate the workflow in the theme, rather than in the buttons. Different coloured backgrounds with a key, maybe a DRAFT watermark as I mentioned earlier. Basically, using the strength that we have content here and we're not bound to just fiddling with forms. That's out of scope here though obviously, but could pave the way for some nice admin-enhanced themes?
Comment #62
jessebeach CreditAttribution: jessebeach commentedRight, I'd much rather see contrib address complex workflows.
Comment #63
jessebeach CreditAttribution: jessebeach commentedThis patch is a shot across the bow to get us rolling. It's very very very very very very very rough. In fact, the only thing you'll see is a red dotted line around the entity when you enable quick edit on it.
This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.
Comment #64
jessebeach CreditAttribution: jessebeach commentedI talked architecture with Wim this morning and we've decided to move forward with an attempt to refactor Create.js out of Edit. The entity-level toolbar designs are not supported by the assumptions in the architecture in Create.js. We would end up writing a lot of glue code to bond these designs to the current codebase and it would be nothing but brittle.
I took the changes from this patch, http://drupal.org/files/refactor-create-out-1966704-11-do-not-test.patch, combined them with the patch in #63 above and based them all on #1818560-40: Convert taxonomy entities to the new Entity Field API. I did this with an approach that could only be described as "code pulverizing". I want to get this patch posted though so that the progression is captured in the issue.
The refactor will take the hybrid jQuery-UI/Backbone approach in Create.js and move it to a pure Backbone solution. Create.js has made sense for our use case up until now, but as we continue to refine the interaction, we must always consider the UX we want vs. the code we have. The nature of agile programming is that each time you touch code, you refactor it to be better and meet your current use case. That's what we're doing here.
Comment #65
MustangGB CreditAttribution: MustangGB commentedI really like that you can edit multiple things on the page without having to click save after each change, hopefully this will work properly when editing multiple pieces of content on the same page, for example two different nodes.
I don't like that when showing the editable areas that the node title, body and tags didn't each have a separate pencil icon and that after clicking the pencil a second click was required to select the specific field to edit. It should just be a pencil icon for each field and clicking it take you straight into editing that field. Or even better not having to click the pencil icon and just click exactly where you want to start typing. A single click, type, save, simples.
I also didn't like that at no point was the question asked "Would you like to save the changes as a draft or just publish the changes immediately?". Perhaps we could use the a workflow that users should already be familiar with and that is one of when saving a new word processing document. The first time an unsaved document is saved the question is asked "Where would you like to save this file to?", then subsequent saves will automatically use this same location. So we could have a similar flow, first time save is clicked ask the question, user edits some more and saves again, don't ask just use same location, user exits edit mode, then next time edit mode is entered again they must again answer the question on the first save.
I don't really mind that the save and close buttons follow the actively editing field around the page, but it would feel easier to use if the save button would just stay in the toolbar next to the edit button and when scrolling down the page the toolbar would always remain visible, pinned at the top. Then clicking save could even automatically take you out of edit mode.
These are what my thought were whilst watching the four UX videos.
Comment #66
jessebeach CreditAttribution: jessebeach commentedThis patch introduces an EntityModel that drives both the ContextualLinksView and the EntityView; it influences the EditableView. The EditableView now has a primary model, the EditableModel, that I am slowly porting from the Create.js widget.
Comment #67
Wim LeersNote that what we're implementing here was usability tested at #45. We will not deviate from what was tested there, definitely not such big conceptual changes.
To actually answer your suggestion: what you're suggesting requires a whole sea of pencil icons/contextual links. That's very, very bad for UX. You start to in-place edit an entity, then you can click on any of the fields to in-place edit them. Period.
Please just bear with us as we implement this; the current patch does not yet fully implement what was tested in #45. The issue was not marked as "needs review", so please save your energy for later, when we will actually need reviews :)
Thank you :)
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commented@Wim Leers, I think you're misreading @akamustang's comment. He wasn't reviewing the patch; rather, he was reviewing the videos. And although one of the alternatives he proposed would result in a sea of pencil icons, his preferred alternative would actually result in removing the sea of pencil icons (or maybe more like a large lake?) that is already there.
I watched some of the videos and participant #3 actually experiences the exact issue that @akamustang described: After clicking Edit in the toolbar, he tries to click on some text on the page to edit it, but can't. However, he does notice the pencil icon pretty quickly after that and figures out to click it, so maybe it's not a huge problem. (Also, this feedback is at least as much related to another issue - such as #1874664: Introduce toolbar level "Edit" mode that shows all contextual links - as it is this one. I have some expanded comments about the videos I will leave on that issue as well.)
***
However, the thing I couldn't figure out from the videos that is related to this issue is that the prototype that was tested doesn't have contextual links at all. So I didn't really understand how the design that's proposed here (for the floating entity-level toolbar with the node title and "full edit" link in it) is supposed to interact with the contextual links (in particular the "quick edit" link, but also any other contextual links that normally show up when you click on the pencil icon). Did I miss where that was discussed?
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedI also have a question on the implementation end: Why is this issue related to #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits at all? (I'm asking mainly because that one seems to have some very thorny problems to solve.)
In other words, why can't the server-side code here just use Drupal's standard node preview functionality to render the updated node and return it to the client side (without ever saving it)?
I haven't looked deeply into the code so I don't know if that's a stupid question, but I'm asking it anyway on the off chance it's not... It seems like it would be a much simpler way to deal with the backend half of this critical task, compared to getting into all the complexity involving TempStore. TempStore is necessary if you care about "oops my browser crashed and I lost all my work and want to get it back" but that's not related to this issue, nor by definition is it that important for something labeled "Quick Edit" anyway :)
Comment #70
jessebeach CreditAttribution: jessebeach commentedThis is another progress-update patch. Expect another tomorrow.
I've managed to get the entities and editables driving view changes from models for each. Activating quick-edit on an entity activates it. Clicking on a field activates the field. Only one of each can be active at a time and we're using change listeners on model collections to enforce this rule rather than keep a list of DOM elements or widgets and then invoking methods to disable them. All disabling happens by manipulating the state of the entity and editable models.
I've managed to cut Create.js out of the widget business. It's still in there for the temp storage, but I don't think this will last long either. My next task is to get an editor and toolbar view to render when an editable is activated. I'm generating the Views for them and they're hooked into the right models. I just need to port the methods and update the rendering. Soon we'll no longer have code like
this.options.widget.options.editor
to decipher.Edit: This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.
Comment #71
jessebeach CreditAttribution: jessebeach commentedThis patch completely removes create.js and the jquery.ui dependency from Edit.
It's still not clean. oh no. There's lots of code to remove. But even now, that stats are looking good.
22 files changed, 2962 insertions(+), 3470 deletions(-)
I've managed to get the editor and toolbar views to appear. Fields can be changed and saved and the save does succeed. I haven't plumbed through the model updates for when a save is finished or when an editable is turned off. All of the code to do that is there, I just need to wire it up to a triggering event and remove some vestigial create.js code.
The file structure can probably be improved, too. Right now most of the classes just live in edit.js. Once the refactoring solidifies I'll break this into smaller files.
Now that we're on a proper Backbone framework and out of the land of jQuery UI widgets, we can start moving on the entity-level toolbar view.
This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.
n.b. that there's a small issue in core with the Editor module. You may need to apply the attached editor-bug.patch for field saving to work.
Comment #72
Wim LeersHeroic work, Jesse! There are no other words for this :)
On the move away from Create.js:
I don't think it's final yet, we're seeing what it would look like if we refactored Create.js out of Edit.
It is almost completely broken for me, but that is to be expected. Saving works, but the re-rendered result does not show up. All of the state handling is broken.
Remarks
Drupal.behaviors.edit.collections.editables
; a collection of all editable fields on the page. An independent field is never editable; a field within an entity is. So I got rid of this collection, and instead created a "fields" collection inside each EntityModel: a list of all editable fields for this entity.Changes in attached reroll
Note that I explicitly omitted the "if the node 1 title changes, then it should also be updated elsewhere on the page" part. It's only relatively rarely seen, and it complicates things. Let's do that in a later stage of this refactoring.
Drupal.edit.Editable
, as you suggested in chat). The "rerender the changed field" part has not yet been implemented. Saving is broken. I focused on fixing the state handling, which needs to work first before we can really fix this properly.Note that:
(for simplified refactoring for now; I'll fix that aspect later)
Comment #73
Wim LeersThe "form" in-place editor now works completely: it loads the form, reacts to modifications in the form, the save/close buttons work, the modal for confirmations works.
Next: make the other in-place editors work, work on getting the saving to work again, explore the use of
Backbone.Model.validate
to implement theacceptStateChange
callback, so I can get rid of theEditableModel.setState(X)
method I introduced in the previous patch, whereas you should be able to just doEditableModel.set('state', X)
.Comment #74
Wim LeersNow using
Backbone.Model.validate
, and hence got rid ofsetState(X)
in favor ofset('state', X)
. Now also ensures only valid states are accepted.(I wanted to post this separately because it's such an important interdiff.)
Comment #75
Wim LeersI still have to work on the distinction of some editors working at the field wrapper level (when cardinality > 1 or a non-textual field) or at the field item level (when cardinality === 1 and a textual field that can be edited in-place). Only then will the "direct" editor work well again.
Jesse, you can already work on getting saving to work again.
Comment #76
jessebeach CreditAttribution: jessebeach commentedThis patch moves the rest of the Models and Views out of edit and into their own files. I cleaned up a lot of comments as well as we start to look towards getting this ready for commit down the line.
I renamed the files to match their class names. Maybe this is not ideal, but it seemed like a good idea at the time.
I'm looking into fixing saving. We've not quite extricated VIE completely. As far as I can tell it's doing a little local storage of value that get push to sync. We can definitely do this on our own. I just need a few more hours to puzzle out what the previous behavior was and make sure I reconstruct it. I'm confident I'll get it solved.
I'm also beginning to work on an entity-level toolbar now that the major refactoring is over.
Comment #77
Wim Leers#76: WOW, that looks *so* much cleaner already!
Turns out I'm too tired to start working on the save stuff, for which I need my full attention. That's for tomorrow morning.
Simple changes here:
findEditableProperties()
Comment #78
webchickMoving to needs work to reflect that there are patches here, but they're definitely not ready for real review yet.
Comment #79
jessebeach CreditAttribution: jessebeach commentedThis patch introduces an entity-level toolbar.
These are the issues I'm trying to wrestle down.
Also, I added some code in the
Drupal.behaviors.editor.detach
method to staunch an error that was arising when I tried to close an editor. I'm hiding an error that should be fixed upstream.When the textarea formats value is
text_plain
, an error is thrown becausesettings.editor.formats
doesn't contain this key. Because the text_plain editor isn't detaching, sometimes you get two editors loading for the same field.Comment #80
webchickJust a question, feel free to say no... Since this patch is pushing 250K and still ain't done yet, does it make sense to split the "refactor Edit module to not rely on VIE/Create" to its own issue, and build the entity toolbar part of it here, with that patch as a dependency? That might also help provide transparency around what's happening, since I'm quite sure from just scanning the issue title, no one would have any idea that the former is what this patch actually did.
Comment #81
Wim LeersAgreed with #80.
I have to run now, but I'll create an issue later today. I've continued working off of #77, because #79 introduces entity level toolbar specific changes. We don't want those in a refactoring, in a refactoring we want to retain the exact same behavior.
Update:
These two points are the last bits of confirmation we need IMHO to 100% justify the move away from Create.js and VIE.
Comment #82
jessebeach CreditAttribution: jessebeach commentedI've moved the Create/VIE changes into their own issue #1979784: Factor Create.js and VIE.js out of the Edit module.
This issue is now concerned with only the changes necessary to introduce an entity toolbar.
Comment #83
jessebeach CreditAttribution: jessebeach commentedProgress update
For the next update
I made a short screencast that illustrated how the entity toolbar shifts to be on screen if a user scrolls it off screen.
http://screencast.com/t/vHWQMTYhR
This patch is rebased on #1979784-6: Factor Create.js and VIE.js out of the Edit module applied to 8.x HEAD.
Comment #84
arosboro CreditAttribution: arosboro commented@jessebeach. I'm just joining this discussion now. How can I apply your patch for #83? Do I need to checkout 8.x HEAD then apply patch #1979784-6 and then apply patch #1678002-83? Do I apply the patch in the directory created with drush make? Thanks.
Comment #85
Wim Leers#84: Please note that this is not yet in a reviewable state. And your first answer (first the patch in the other issue then this one) is the correct one.
Comment #86
arosboro CreditAttribution: arosboro commented@Wim Leers,
Thanks I'll work on other issues in the meantime and follow this thread until it's marked for review.
Comment #87
jessebeach CreditAttribution: jessebeach commentedRerolled on
#1979784-10: Factor Create.js and VIE.js out of the Edit module
#1901100-23: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
applied to 8.x HEAD (891f4c5d6928d122bc1b255327f7077599f3eab3).
Hi arosboro! I'm psyched that you're interested in review this patch. I agree with Wim Leers that it's still quite messy. I encourage you to jump in whenever you feel comfortable with the setup and state of it. Right now we're building a few interrelated patches which means we're constantly rebasing them against each other and against D8 HEAD, which itself is moving quickly. Each time I post a patch here, I try to indicate exactly what patches and version of D8 HEAD I created it on, so you can recreate the context for the patch without running into apply conflicts.
Comment #88
Wim LeersOnce this is complete, we should also make this functionality accessible: #1844220-15: Make in-place editing keyboard and aurally accessible. Or maybe that can or should be done as part of this issue, that's TBD.
Comment #89
jessebeach CreditAttribution: jessebeach commentedRerolled on
#1979784-12: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #90
jessebeach CreditAttribution: jessebeach commentedThis patch allows for entity to be saved. Once a field is changed and saved, the save button can be clicked again to save the entity.
Current issues (among many)
Comment #91
Wim LeersPermanent simplytest.me link to test #90 (which needs #1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits and #1979784-12: Factor Create.js and VIE.js out of the Edit module): http://simplytest.me/project/drupal/be3667b3efc81bbe11e2226e1f92eec1db89631a?patch[]=http://drupal.org/files/edit-tempstore-1901100-24.patch&patch[]=http://drupal.org/files/edit-create-refactor-1979784-12-do-not-test.patch&patch[]=http://drupal.org/files/entity-toolbar-1678002-90-do-not-test.patch
I confirmed this to be working. Nice work, Jesse :)
Comment #92
klonosI gave it a quick spin and had a couple of issues (Win7 x64 on firefox x64 nightly). These issues would have been more obvious if only I had taken a video screencast...
I created a simple article node with a "test article" title, a "test" tag and a "test body" body. Then, while in view mode, I clicked on the contextual pencil icon and selected "Quick edit".
- Hovering over the tag and body displayed the floating toolbar with a proper label for each of the tag and body fields.
- Clicking on the body field expanded the toolbar to display the buttons, but it was actually hiding the body text underneath it. I couldn't edit it.
- Clicking on the tag field brought the field in edit mode but it also enabled the "save" button even without having typed anything.
- The tag field height was a few pixels taller than it should, so it showed part of the blue spinning throbber along with the gray static one.
- I was locked in the edit state with the toolbar disappeared and I couldn't click out of the tag field edit mode.
Comment #93
Wim Leers#92: from #90:
This patch is *not* marked as "needs review", so please don't waste your time on doing that :) We're very much aware that this is is still very rough and buggy :)
Comment #94
jessebeach CreditAttribution: jessebeach commentedklonos, we'll very much appreciate your detailed eye for review when this code is working. Look for it to change to needs review and then go crazy!
Comment #95
Wim LeersYes, that's what I tried to say but did so pretty poorly :)
Comment #96
klonosYes, I failed to see status. My bad.
Comment #97
jessebeach CreditAttribution: jessebeach commentedThis patch further moves state management of the fields into FieldModel. This allows the EntityModel to get a list of 'editing' fields like this:
What it means to be an editing field is determined by the FieldModel. EntityModel just wants to know if the field is in an 'editing' state which in this case means any of the following states:
['activating', 'active', 'changed']
Built on D8 (50ae3296382669baebee8b9afa909361eb5aa412) with following patches applied:
#1979784-12: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #98
jessebeach CreditAttribution: jessebeach commentedThis patch cleans up the positioning of the . I took a hint from #1979784-16: Factor Create.js and VIE.js out of the Edit module and I'm now positioning against the
EditorView.getEditedElement()
element when it exists. This is the field element being edited and not the wrapper around it, which is what thefieldModel.el
attribute points to.After chatting with Wim, I determined that I was going too far with the changes I was introduces, so I paired them back. We're now targeting a minimally changed design in this issue that introduces an Entity Toolbar with minimal changes to the underlying behavior of inline-editing.
Still left to do:
Built on D8 (686c269bae7de4d84d4dd241c9b9f9092ece4f01) with following patches applied:
#1979784-18: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #99
lightsurge CreditAttribution: lightsurge commentedShouldn't 'Save' in the toolbar actually always save all changes i.e. save the complete entity?
Seems to make sense that changes to fields can be 'confirmed' by simply moving between fields, and it's not necessary to have a button for it. So if I change a field, and click in the body, the changes to the field are 'confirmed'. If I click on a completely different entity thinking I can continue working on the page without yet saving, I should perhaps be asked if I wanted to save the previous entity, or whether to cancel all changes?
If anything rather than confirming or cancelling changes to a field, a user may desire fine-grained control over undoing individual changes, so the X in the toolbar could be relabelled 'Undo last change', with another button 'undo all changes' that scrubs all changes to the entity. I guess with the way this is working, it would even be possible to undo a number of sequential changes, but 'undo last' or 'undo all' seems a reasonable starting point, if it's needed at all for good user experience.
I think as far as 'saving' goes, it's actually quite a nice safe button that will give people confidence, so long as as with traditional offline editors it is 'governing' the complete entity (or the whole page), and we may lose that confidence by befuddling it by adding a curve ball in the form of an between-fields 'confirm changes' button.
Comment #100
lightsurge CreditAttribution: lightsurge commentedRealise it sounds like I'm saying this whole issue is misguided... I'm not! Just trying to point out that the 'entity-level toolbar for saving fields' shouldn't have to be about literally saving fields, but some form of interface for confirming or not confirming changes.
I just don't think the 'confirming' is entirely necessary, because if you don't want to confirm it, you can 'unconfirm' it by clicking undo. If people have made changes, they've made changes, they don't know that these changes haven't yet flowed into a temporary store that's the precursor to being fully saved. So if they've made those changes, if they don't like them they click 'undo', if they're happy they continue editing.
Hope that makes sense.
Comment #101
lightsurge CreditAttribution: lightsurge commentedThis would be ideal as I see it:
3 buttons.
1. Save - present if there are atomic edits in tempstore, or a field is activated and changed
a) If a field is activated and changed, confirm the change and save all atomic edits in tempstore
b) Otherwise if no field is activated and changed, save all atomic edits in tempstore
2. Undo last - present if a field is activated and changed, or there is an atomic edit in tempstore
a) If a field is activated and changed, revert the field to that saved
b) Otherwise clear last atomic change in tempstore and undo the change or the whole entity if only 1 change
3. Undo all - present if there is >1 atomic edit in tempstore, or field is activated and changed and there is 1 atomic edit in tempstore
a) Clear tempstore and revert the whole entity
Comment #102
jessebeach CreditAttribution: jessebeach commentedlightsurge, these are exactly the issues I've been struggling with here. As we move from a per-field editing experience to a per-entity editing experience, it's right to point out that the interaction should change. A user would expect to move between fields easily and then commit those changes once by saving the entity. And then, as you point out, once we allow changes to stack up, we also need a way to unwind/rewind that stack i.e. undo/redo.
We will most likely need to address these changes in stages. The first here is to introduce an entity toolbar that just barely alters the current field saving behavior. We know the current behavior is functional. The entity toolbar has enough complexity that introducing it is a considerable coding effort. Changing the UX of in-place editing in a way that meets the expectations you describe above would become a follow-on effort. There's also the current architecture of Drupal to consider. We're working on #1979784: Factor Create.js and VIE.js out of the Edit module and #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits in tandem to improve our ability to model an entity on the front end and edit it atomically on the backend with some kind of temporary storage layer in between. None of this, of course, precludes us from defining what the ideal experience should be.
I'll chat with Kevin and see if we can put together a vision of what the short term goal for this issue looks like and what a longer-term, entity-based.
lightsurge, do you feel like you could mock-up the ux you describe above in some images?
Comment #103
Bojhan CreditAttribution: Bojhan commentedI don't feel like we should expand the scope of this issue to include "undo", although yes - if you are moving across fields with temp storage - you might want this. I feel like that is a reasonable UX trade off for getting this into core, I'd like us not to complicate this issue much further. If we wish to do this a separate issue should be opened.
Given that it took a lot of time, in prototyping, discussing and finally data, to arrive at this direction - lets not deviate before we have a fully working patch that we can actually evaluate. As previously stated the work here, is very much work in progress - I dont think we can fully evaluate it, untill we have an actual working patch (right now its too bugged, to really evaluate as pointed out in #94).
Comment #104
Wim LeersBojhan++ :)
The feedback is well-intended, but this is just the wrong time :)
Comment #105
lightsurge CreditAttribution: lightsurge commentedThat's fair enough, this was in response to #98 with the meaning/context problem in terms of 'how do we make the save button make sense now'.. but if my feedback would be a deviation rather than a possible solution, I'm +1 for something else as well ;-)
Comment #106
Bojhan CreditAttribution: Bojhan commented@lightsure No worries, I just want to make sure, we get to the point where we can evaluate it fully. The problem you identified, exists - but it might be better easier to find solutions to once we have a good baseline interaction. I'd also be a better point of determining whether its part of this issue, or whether it should be a separate one.
Comment #107
jessebeach CreditAttribution: jessebeach commentedOf the things left to do listed in #98, here's what this patch addresses:
These have been addressed, but with caveats.
And I haven't addressed this one yet.
The big change is that you can now start quick editing, change a field, click into another field without clicking the save button, and your changes from the previous field will be saved to tempstore. Clicking the save button writes everything to the database.
Built on D8 (75c407547fab8db09022479facf69460ab096b92) with following patches applied:
#1979784-18: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #108
falcon03 CreditAttribution: falcon03 commentedSo I think this patch is not ready for an accessibility review, is it?
@Jessebeach, you wrote:
I didn't review the patch at all, but the toolbar should convey this information also without relying on colors to be accessible. E.G. Using colors and a (eventually invisible for sighted users) text.
Comment #109
jessebeach CreditAttribution: jessebeach commentedIndeed falcon03, what I should have written is: "When a field is changed and 'saved' but the entity hasn't been saved, we need to indicate in the markup that it's dirty."
This patch isn't ready for accessibility review, but it is definitely ready for accessibility development.
Comment #110
jessebeach CreditAttribution: jessebeach commentedThis path should clear up the issue #2 for comment #107
The save button issue is also cleaned up:
The last thing I need to introduce is the full edit link that brings the user to full node editing. I foresee some difficulty transferring data from edited fields to the full edit node form. If this takes me more than 45 minutes to solve, I'm going to drop it and push that behavior to a future issue in favor of getting something reviewable completed this weekend.
Comment #111
jessebeach CreditAttribution: jessebeach commentedThis is a rebase on top of the following patches.
#1979784-30: Factor Create.js and VIE.js out of the Edit module
#1901100-25: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
There are numerous regressions from #110, but the patch at least applies and given the number of code conflicts, this is a definite improvement. The regressions should be easy to hunt down. They include.
Comment #112
Wim LeersPosting an initial reroll already, because it fixes #111.1 & #111.2 — it's fully functional again.
(This is not strictly a bugfix reroll, it's part of my review process, so there's also changes in there unrelated to solely fixing the problems pointed out in #111.)
Changes
CustomBlock
s). I made it work for all entities. I also renamed it from "entity title" to "entity label" everywhere, because that's what it's called in the PHP code too. Also updated the interface.::
. That's hard to parse. I modified it a little bit, but of course we need to refine this later on.drupalSettings.edit.entitySaveURL
is not used. That's fine (even better, actually: less settings to pass!), but the path is hardcoded to be root-relative, even though Drupal may be installed in a subdirectory. So now it's usingdrupalSettings.basePath
.EDIT: forgot to say: CKEditor's toolbar doesn't show up yet, but it *does* load. So it's definitely not yet fully fixed.
Comment #113
Wim LeersAnd now the CKEditor toolbar also shows up again — everything is working as in #111 again AFAICT.
Comment #114
Wim Leers(This is the last small update, I promise.)
Until now, the
!reset_tempstore
part of the URL was not being set properly (it's a mystery to me how this ever worked before — the parameter wasn't being parsed on the server side nor was it being set on the client side). This moves that from the path to a POST parameter, and ensures it's set correctly when appropriate. Only now the queueing of changes and clearing of changes will work correctly. As soon as at least one change made within the current page load is stored in the TempStore, then we'll just keep on queuing.Changes
!reset_tempstore
part of the URL is not being used at all! As a consequence, it's impossible to queue multiple field changes and save them all at once.(I know I'm stepping on #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits's territory here, we can move changes around as is appropriate later.)
Comment #115
Wim LeersReview
FieldToolbarView
be removed altogether? Doesn't it make more sense to letEntityToolbarView
also take on its tasks?_ignoreHoveringVia
implies that if you hover away from a field onto the toolbar (which used to be the field toolbar and now is the entity toolbar), that will in fact count as a mouseleave. We want to allow for hovering the toolbar, because otherwise the label in the entity toolbar is ADHD-like :)inactive
state instead of first going to thecandidate
stageEntityModel.attributes.state
is intended to "mirror the state of the currently being edited field"? But at the same time, it is not being used at all AFAICT? Can't this be removed?I think
EntityModel.attributes.is(Active|Dirty)
are sufficient?EntityModel
's listening to theviewChanged
event, which calls theviewChange
method, which triggers thefieldViewChange
method on theEntityToolbarView
, which in turn triggers therender
method… can't that be simplified? No matter the answer, the names should become more descriptive than "view changed" :)EditorDecorationView
sets theedit-changed
class, but never removes it, it'sAppView
that removes it. The same object should be responsible for setting and removing it. Furthermore, in the case of closing an entity without saving, this is why those fields remain marked as changed afterwards.AppView.enableEditor()
, special care is taken if the previously active field's state ischanged
. But the similar care should be applied when the state isinvalid
: that's also a changed field, that was attempted to be saved, but turned out to be invalid. So the user should get the choice: modify it to be valid and save again (i.e. disallow the user to go to another field), or discard the current value. This is the only case where it's acceptable to have a modal in between moving from field A to field B, AFAICT.AppView.enableEditor()
, I'm missing a comment to explain that it's okay to set the field whose editor will be activated to theactivating
state immediately, without first setting it tohighlighted
. The reason being that the current state will already behighlighted
, because you had to click (and thus hover) it first. The point is: we should explain that we're not skipping any states!We should never allow a field to transition directly to the
inactive
state. All code is currently built on the assumption that a field first must go tocandidate
before it can go toinactive
. We can change that in the future, but not in this patch IMHO.Related to that is this change in
formEditor.js
:AppView.rerenderedFieldToCandidate()
is making DOm changes that should happen inEditorDecorationView
.AppView
should never modify the DOM directly.display:inline
fields are broken:First,
this.$field
doesn't exist anymore, second, the positioning is gone. Quite possibly the positioning isn't needed anymore becauseFieldToolbarView
is contained withinEntityToolbarView
. But doesEntityToolbarView
already supportdisplay:inline
elements?EditorView.stateChange()
is absolutely unacceptable:This is most definitely the wrong place to be doing this.
stateChange()
'soptions
parameter intoEditorView.save()
, but I don't see any other way. It should be explicitly documented onEditorView.stateChange()
though that it is essential that thesave
method mustoptions
.aria-hidden=false
as soon as the entity becomes dirty :)AppView.save()
needs to be simplified.Changes
EntityModel
(where they are) nor in theFieldCollection
(where the comment said they should be moved), they belong in theAppView
. Since only the pre-existing one inAppView
wasAppView.changedFieldsInTempstore
was still usingfieldModel.get('editID')
instead offieldModel.id
.Comment #116
jessebeach CreditAttribution: jessebeach commentedI'm not sure what an "inline toolbar" is. I think this is what gets rendered on a plain text field, right? When the editor is rendered in overly, e.g.
I've never quite understood why this mode of editing is necessary. In other words, why doesn't the editing happen like it does with CKEditor?
Comment #117
Wim Leers#116:
display:inline
fields that exist in core are the author and date fields on nodes — in that case, the toolbar needs to be positioned in the right location using a different approach. Although with the entity toolbar, that might no longer be necessary, as I said.Comment #118
jessebeach CreditAttribution: jessebeach commentedOk, I think we can pull the inline distinction out then. The EntityToolbar positions against the edited element of a field.
Comment #119
Wim LeersSimplification! Awesome :) Very, very happy to be relieved of that edge case!
Comment #120
jessebeach CreditAttribution: jessebeach commentedI took the changes to fix tempstore in #114 and moved them to #1901100-27: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits.
This patch also fixed the contextual visibility. It is now based on an active entity, not an active editor.
Rebased on D8 3ad81ec0a6b8cba88cabd0b422c3f9cd5f750f91.
Built on
#1979784-36: Factor Create.js and VIE.js out of the Edit module
#1901100-27: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #121
jessebeach CreditAttribution: jessebeach commentedThis address Wim's first comment in #115:
Rebased on D8 3ad81ec0a6b8cba88cabd0b422c3f9cd5f750f91.
Built on
#1979784-36: Factor Create.js and VIE.js out of the Edit module
#1901100-27: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #122
jessebeach CreditAttribution: jessebeach commentedI don't think this is the case. When an editor is active, the toolbar always reflects the label of that field. Otherwise, when you're hovering a field, the label reflects that hover.
When an editor is not active and you hover from a field to the entity toolbar, the field label disappears and just the entity label is present because at this time, you are acting on the entity, not a field, if you press save or close.
Comment #123
Bojhan CreditAttribution: Bojhan commented@jesse I am slightly confused, I have been following this perhaps misunderstanding for a while but from my point of view, there won't be a entity toolbar active unless you are editing a field. When you are not editing a field, there is no reason to have an entity toolbar? All we need is some signalling when you go out of "quick edit mode".
Comment #124
jessebeach CreditAttribution: jessebeach commented@Bojhan, the entity toolbar will be present while an entity is in "quick edit mode". The actions available in the entity toolbar will be dependent on the field that is actively being edited. When no field is being edited, the entity toolbar will provide a save action (if a field has been changed) and a cancel action to quit "quick edit mode".
Comment #125
Bojhan CreditAttribution: Bojhan commented@jesse I get that the patch currently does this, but where was this design decision made. And more importantly why? It is my understanding we didn't want a "hovering" bar, when you are outside of editing a field because;
1) its not really relevant when you are "overviewing"
2) we want to push towards clicking "save" when you are editing your last field
3) the bar would hover over other elements, potentially obstructing the view to parts of the entity? In the case of editing a block/menu inline
I do see a lot of advantages too, but I'd like to know when and how this decision was made because this was not reflected in the prototype and/or design that we agreed upon in the direction provided in #39 and #41. I have been trying to follow this thread, but failed to spot this UX decision.
Comment #126
Wim Leers#125: That's how the prototype works: http://www.acquians.info/Public/Edit/spark/blocks/index.html :)
Comment #127
Bojhan CreditAttribution: Bojhan commented@WimLeers Hmmpf, my mistake. I thought that was simply because it was still a raw prototype. I will have a chat with Kevin, because the design will need to be adjusted if we keep this in - right now (in the prototype) it looks like you are always on the top field - even outside of edit mode.
Comment #128
Wim Leers@Bojhan Maybe we can do a quick call so I can explain this? I still think there is some misinterpretation going on.
Comment #129
jessebeach CreditAttribution: jessebeach commentedThis patch contains some experimental code exploring how we deal with the managing the state of the entity as a function of the state of its fields. An entity's fields' states determine if the entity edit session can be closed or opened. We're running into race conditions, so more work is needed.
Built on ff6e1eea6ea2b2b343a4e03e172e6f5a05079dcb
#1996378-6: Edit broken because of #1043198 and routing system bug + missing test coverage
#1901100-29: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits, altered slightly to deal with a conflict in EditController because of changes in 1996378-6. This will be resolved as soon as this dependent patch is committed to head and we make a reroll of 1901100. Don't worry about it now. The code here is just for posterity.
Comment #130
jessebeach CreditAttribution: jessebeach commentedThe following scenarios should now be supported.
Save
Cancel
Change field > Cancel > Save
Change field > Cancel > Cancel
Currently, only text fields are saving. Image fields and tag field changes are not saving. This appears to be server-side errors, but my bet is it's something we're passing to entity save, since a full node edit works just fine.
Also, mousing off the entity toolbar makes the save button disappear. Small issue, will be addressed in the next patch.
Built on
6c1c3b9b96c3c31bb761e6d4dc56a15bb3eaa061
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #131
jessebeach CreditAttribution: jessebeach commentedThis code contains comments from my discussion with Wim at DrupalCon Portland. It doesn't push functionality much farther than #130. I wanted get this code off my computer though and made public because I will not be able to focus on it for the next 2 days.
Built on
0979e1f5876a3d529320cceaede1770bda425ed8
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #132
jessebeach CreditAttribution: jessebeach commentedRerolled because of HEAD conflict.
Built on
affc74ccddbd92b88d9fb23756a0c20df4b19a2c
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #133
jessebeach CreditAttribution: jessebeach commentedI fixed the disappearing save button issue mentioned in #130
I've also begun to address treatment of invalid field data. Most of the pre-refactor code worked just find. I just need to wire the field states up to the entity toolbar properly and change some CSS. This was a lot less work than I had predicted.
With the major pieces of functionality in place, I'm going to start writing tests for this feature into the FAT module.
Built on
ace3459c4bd3ad50db8d49b700dde202d01e419d
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #134
jessebeach CreditAttribution: jessebeach commentedI've gone through the code and cleaned up the CSS and JSHinted the JavaScripted. All functions that previously had no or insufficient docblocks should be marked up sufficiently now.
I've also gone through the patch line by line and cleaned out any crufty leftovers from development up to this point. From here out, we're working towards commit.
I've written tests in the FAT module, http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
These are some of the issues I'm still seeing.
Comment #134.0
jessebeach CreditAttribution: jessebeach commentedUpdated issue summary. Incorporated #1741692: Discard via ESC button.
Comment #135
klonos:D
Comment #136
tkoleary CreditAttribution: tkoleary commentedCouple of things I noticed:
Bug: Quick edit gets repeated multiple times in the dropdown
Bug: Textarea with only one line of text. Wysiwyg toolbar covers the text on focus.
Bug: Overlapping field borders
See attached images
Comment #136.0
tkoleary CreditAttribution: tkoleary commentedUpdating the issue description
Comment #136.1
jessebeach CreditAttribution: jessebeach commentedtesting.
Comment #136.2
jessebeach CreditAttribution: jessebeach commentededit
Comment #137
jessebeach CreditAttribution: jessebeach commentedI realized while testing that reverting changes does not work because placing a field in candidate state destroys its EditorView. This happens when
Making edits to a field, canceling from the entity toolbar, then canceling the confirmation dialog.
Making edits to a field, switching to another field, canceling from the entity toolbar, then canceling from the confirmation dialog.
The first scenario can be fixed, perhaps, by tweaking the flow of how when we call EditorView.revert in the entity edit session shutdown process.
The second scenario leads me to believe that we can't destroy the EditorView on a field once it has been established, because we need something that will know how to revert the changes even when that field no longer controls the active editor on the page.
Built on
56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-45: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #138
Bojhan CreditAttribution: Bojhan commented@jesse Could you please mark it "needs review" when the usability team can look at it? Please do it before you make critical UX decisions, so we can review it.
Keep up the good work, its great to see this moving :)
Comment #139
jessebeach CreditAttribution: jessebeach commentedDefinitely Bojhan. I wish I could make this move faster. I've very sensitive to the fact that this UI feature has data-loss potential, so getting the internals right is causing me some grief. Wim and I are going to discuss the current state today.
Any help filling out the manual testing scenarios in the issue description would be much appreciated. There are so many paths through this UI. I want to be sure we haven't missed any of them.
Comment #140
Wim LeersAssigning to myself to prevent further patch rerolls while I work on a comprehensive review, along with a reroll to address some things that I already can address.
Comment #141
Bojhan CreditAttribution: Bojhan commented@Jesse Awesome, thanks! I totally understand this is a big effort, looking forward to seeing the result.
Comment #142
Wim LeersOverall: awesome job :)
I cleaned up a lot of small things, added docs, fixed some subtle but fundamental problems, and ensured the fluent UX (always allow clicking from one field to the next) we want is really there.
TODO
EntityModel.save()
, but it's not fully there yet. (time constraints)Drupal.ajax
to make it work for concurrent use cases (which we need for fluent UX, see below), but it seems that if a field that uses the "direct" editor (a plain text field) that is required is first mad empty, to trigger the 'invalid' state, then is filled in again and then saved, causes the submit to be an *regular* submit rather than an AJAX submit; result: AJAX barf presented to the user.Code review
edit.js
data-entity-id
attributes also lack the language identifier. The difference is that before, this was just for internal use, but now there is metadata so now it *should* include the language code.formEditor.js
@todo This reset param shouldn't be associated with a single field commit. I can't quite figure out how it works yet, though
.removeForm()
when entering the 'inactive' state. I checked with Jesse and this was just a leftover from earlier.EntityModel.js
s/tempstore/TempStore/gi
:)EntityController
and anEntityFieldController
is a nice-to-have refactor. It's not essential. Removed the TODO.isDirty
attribute onEntityModel
andFieldModel
conflated things. Lots of subtle bugs ensued. I've split this intoisChanged
andinTempStore
onFieldModel
, the OR'd result determines whether the field gets theedit-changed
class. Similarly, I've split it intoisDirty
andinTempStore
onEntityModel
. The former works identically to theisDirty
attribute that you had introduced (but it now has corrected docs) and the latter is used to determine the value for thereset
POST parameter. All relevant docs have been updated to be very specific.EntityModel.attributes.changedFields
(now renamed tofieldsInTempStore
attribute which is more accurate), since it is derivable at any time from an entity's fields'inTempStore
attribute. The only complication here is that whenever a field is rerendered (i.e. when it is saved to TempStore), the correspondingFieldModel
is destroyed and hence the field-levelinTempStore
attribute's value is also lost. A solution would be to keep a field'sFieldModel
forever, also after rerendering.So, I started implementing this ("keep
FieldModel
s around forever"), but found several problems:FieldModel
that exists forever but no longer corresponds to a field, which makes no sense; and would likely lead to more problems down the roadFieldModel
instance around implies that the associated element must be updated, but that in turn means that this updated element must propagate to all views attached to theFieldModel
: theEditorView
, theFieldToolbarView
, and so on. It's possible, but it's a lot of extra handling.So, point 2 says it's hard but not impossible, point 1 gives a very good reason to not do it. Overall, it gains us very little: it makes sense that whenever a field was successfully saved to TempStore and is thus rerendered, that all related Views are also purged and rebuilt; the state is the initial clean state again and can be advanced to the target state (i.e.
state = 'candidate'
andinTempStore = true
) very easily. So let's stick to that for now, until we have plenty of free time to refactor, if it still makes sense then :)EntityModel.fieldStateChange()
was changing the state of fields that had just entered the'saved'
state to the the'activating'
state. Not only is this wrong for multiple reasons, it also was effectively a no-op: the field state change validation system prevented this from happening… :) Now I just removed this piece of code.EntityModel.validate()
was validating the *current* state of the entity, not the state it was trying to be set to.FieldModel
's. This addresses these two TODOs:EntityModel.save()
was performing AJAX requests multiple times. Introduced a mutex to mitigate that:EntityModel.isCommitting
. This also removes the hack to useBB.validate()
wheneverEntityModel.save()
is called.EntityModel.states
.FieldModel.js
isDirty
attribute that you added is not updated byFieldModel
itself, but byEntityModel
. BUT: more importantly: is it really necessary? I doubt it is. There already isEntityModel.changedFields
: why not just use that instead?AppView.js
EntityToolbarView
instance atEntityModel.attributes.entityToolbar
(i.e. as a Backbone Model attribute), store it as a property on the object itself. Which one is better doesn't matter at this point, it's just for consistency with how Views related toFieldModel
are handled.Much like the same assumption in
formEditor.js
. Also checked with Jesse: also leftover, also undone.EditorDecorationView.js
EditorDecorationView
was calling intoAppView
, which is unacceptable:Also, this should be handled by the state handling code, the calling code shouldn't be aware of application details, it should only declare what state it wants to achieve.
To solve this, the original LoC was restored, I modified
AppView.acceptStateChange()
to embed the logic that lived inAppView.enableEditor()
and removed that method. I then discovered that the cause and solution to #134.3 was actually right there: when the active field is in the'changed'
state, then its state is changed to'saving'
, but the field that tried to get to the'activating'
state would not be enabled. I fixed that now.By doing that, I've achieved the fluent UX we want, BUT:
'invalid'
state), so we need to handle that too. I implemented this.Drupal.ajax
's documentation is *utterly wrong*, which led to nightmareish race conditions because we can now have two concurrent AJAX requests that result inDrupal.ajax
handling the response. The example provided byDrupal.ajax
overrides the *prototype implementation* of the AJAX command, so it affects all otherDrupal.ajax
instances too! Hence our code did that too. It's pure luck that we didn't run into this problem before, it's the concurrency that surfaced this. I spent at least 4 hours debugging this._pad()
and_unpad()
methods were modified incorrectly, hence the problems :) I think the changes were leftovers from the early days of this patch, where these functions were acting on the wrong element.EditorView.js
<?dt>EntityToolbarView.js
FieldToolbarView.js
util.js
reset
POST parameter that is used inEditController::fieldForm()
to decide whether to render forms based on the entity stored in TempStore or not was not being passed on inDrupal.edit.util.form.load()
, causing an unfinished previous in-place editing session's changes to show up in forms in the current in-place editing session.Built on
56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-47: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #143
jessebeach CreditAttribution: jessebeach commentedI don't understand this comment. Wouldn't the original commands object be overridden by the object literal? You would want something like this, no?
Comment #144
tkoleary CreditAttribution: tkoleary commented@Wim Leers
It's really coming together! Two things I noticed.
Comment #145
jessebeach CreditAttribution: jessebeach commentedKevin and I had a long discussion about this and we've decided to table this detail for further thought. Essentially, a user would see a border around the entity when quick edit is enabled. Upon clicking an editable field, the border around the entity is removed and borders around all the editable fields are revealed. We noted two problematic cases under this model:
Comment #146
jessebeach CreditAttribution: jessebeach commentedI fixed a very minor JS issue that was throwing errors when the entity went into the committing state. See the interdiff. I had to reroll #1901100 and then I ran into a 500 error upon trying to save the entity. I have no idea what to do with this error and it's too late where I am to start digging in tonight:
Uncaught PHP Exception InvalidArgumentException: "No check has been registered for access_check.edit.entity" at /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/Access/AccessManager.php line 193
The error above might need to be addressed in #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits.
I'm still processing the behemoth review/refactor from Wim in #142
Built on
56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #147
Wim Leers#143: the docs were lying before, what they claimed was not actually possible. That's why I tweeted "Once again, I'm fighting Drupal.ajax. This time because it can't deal with concurrency.
Dozens of hours lost. Sigh." — https://twitter.com/wimleers/status/344959451956142080.
I talked to nod_ about this and instead of the work-around that I implemented, he's actually fixing it right now: #2019481: JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides. So, rerolling #146 on top of the patch there that makes the docs no longer a set of frustrating, time-sucking lies :)
#145.2: indeed, it's a too constraining assumption! It might be very useful for UX/clarity, but it's an assumption we cannot make.
#146: good catch :)
Attached reroll removes our
Drupal.ajax
mess work-arounds and instead relies on #2019481: JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides for fixing it.Comment #148
Gábor Hojtsy@jessebeach: I tried to reproduce the "No check has been registered for access_check.edit.entity" issue with #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits but neither manually going through creating a node, in-place editing and calling the save callback reproduced that, nor running the tests for it did (with latest head). The access_check.edit.entity is a service registered from edit.services.yml and associated with a class that has access checking implemented.
Comment #149
jessebeach CreditAttribution: jessebeach commentedRe: #148, I reinstalled and refreshed everything. The patch from #147 does not have the issue I raised in #146.
Reroll
cd8a251f5fe51f888c8df9c48efb6e771c6b2d4a
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #150
jessebeach CreditAttribution: jessebeach commentedThis patch cleans up the placement of the toolbar. It now won't get lost under the toolbar. It also positions itself against the active form editor in addition to the active field, the highlighted field and the entity.
I made a short video of the behavior: https://www.facebook.com/photo.php?v=988762275316&set=vb.501857&type=2
I altered the colors of the buttons to match the Seven button coloring styles.
I also had to add an exception case to the entityModel state change validation in order to allow the transition from changed to canceled to confirm-saved.
Otherwise the entity won't save from a confirmation dialog.
Comment #150.0
jessebeach CreditAttribution: jessebeach commentededit
Comment #150.1
jessebeach CreditAttribution: jessebeach commentedAdded manual testing results to the scenarios in the issue description.
Comment #150.2
jessebeach CreditAttribution: jessebeach commentednotes
Comment #150.3
jessebeach CreditAttribution: jessebeach commentedadded invalid test cases
Comment #151
jessebeach CreditAttribution: jessebeach commentedThis patch resolves an issue with the _pad() and _unpad() methods. They had been called under unwanted circumstances, which would cause fields to appear to crawl across the page. Now, _pad() can only be called once until _unpad() is called.
When canceling changes and then canceling the confirmation dialog box, the page refreshes in order to get the original values of the fields back on the screen. This isn't an ideal solution, but it's the cheapest one at the moment. What we really need is a refactor of the fieldModel rerender pipeline, but that isn't something we should try to tackle in this patch.
And sometimes you'd get a stray 'edit-highlighted' class when closing an edit session. This is fixed.
The flows concerning invalid fields are not functioning well again. Fixing those flows will be the final bit of functionality to correct before we can start looking at this patch for commit.
Comment #152
tim.plunkettThis was tagged critical for being a "feature regression", which can't be true since this was a feature addition to D8.
Comment #153
Wim Leers#1363112: Simplify names of "element-x" helper classes broke #151 in a very minor way.
Review of #150: looks good — I noticed that the positioning is now much more smooth overall :)
Review of #151: the page reload logic was flawed: it was not being reset to its initial state when starting to edit a new entity, plus it was not using
.revert()
when it could. Fixed that, and clarified the comment.I'm still working on a fix for
I'm getting closer, but I don't see the solution just yet.
Comment #154
Wim LeersYay, fixed
! :)
The cause: old
Drupal.ajax
instances used byDrupal.edit.form.ajaxifySaving()
weren't being GC'd by the JS engine, because the jQuery event handler that triggers them contained a reference to them. Said jQuery event handler was not being unbound. Simply unbinding the jQuery event handler hence solved the problem.Comment #154.0
Wim Leersremoved a strong tag
Comment #155
jessebeach CreditAttribution: jessebeach commentedAdding images to put in the description block.
Comment #155.0
jessebeach CreditAttribution: jessebeach commentedadded another test case
Comment #155.1
jessebeach CreditAttribution: jessebeach commentedadded some images
Comment #155.2
jessebeach CreditAttribution: jessebeach commentedupdating manual tests.
Comment #155.3
jessebeach CreditAttribution: jessebeach commentedupdated manual tests.
Comment #155.4
jessebeach CreditAttribution: jessebeach commentedUpdated manual tests.
Comment #155.5
jessebeach CreditAttribution: jessebeach commentedadded more tests
Comment #155.6
jessebeach CreditAttribution: jessebeach commentedupdating tests
Comment #155.7
jessebeach CreditAttribution: jessebeach commentedupdating tests
Comment #155.8
jessebeach CreditAttribution: jessebeach commentedupdating.
Comment #155.9
jessebeach CreditAttribution: jessebeach commentedupdate
Comment #155.10
jessebeach CreditAttribution: jessebeach commentedtest updates
Comment #155.11
jessebeach CreditAttribution: jessebeach commentedupdated tests
Comment #155.12
jessebeach CreditAttribution: jessebeach commentedupdated tests
Comment #156
jessebeach CreditAttribution: jessebeach commentedBetween #154 and #156, I added manual test cases 16 through 20. All of them failed initially. I have them all passing manual tests now. With test #3 (image save) resolved by the commit of #2004350: Unserialised database connection objects don't use Drupal's PDO Statement class, we now have full manual test passage on this feature. I'm happy to open it up to code and ux reviews.
I made a video that illustrates how the in-place edit features looks and responds: http://www.youtube.com/watch?v=FA050kmyPZY&feature=youtube_gdata_player
You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files...
You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.
This patch was built on:
712fe2a18dce3581d3301b43777ac6749ca6c502
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #158
jessebeach CreditAttribution: jessebeach commentedI discovered some issues with formEditor form changes when those changes come back as invalid. The issues are resolved with this patch.
Comment #159
jessebeach CreditAttribution: jessebeach commentedsetting to needs review.
Comment #160
jessebeach CreditAttribution: jessebeach commentedOh forgot to add rebase info and testing info to #158
You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/edit-tempstore-1901100-48.patch&patch[]=https://drupal.org/files/entity-toolbar-1678002-158.patch
You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.
This patch was built on:
5f67fd1d0858c6f52b4f2685a45b673d6b33e449
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #162
jessebeach CreditAttribution: jessebeach commentedAfter reviewing with Dries and Kevin, they pointed out that the contrast between the toolbar (formerly with a white background) didn't contrast enough with other elements on the page to pop out. So I've added some coloring. It's probably not the right color, but it suggests what the toolbar would look like with a better color choice.
The toolbar also now has a little point that points to the field or entity that is currently its context.
I'm not sure if this is desired, but the comments and comment form were quite confusing during an entity inline editing session. The bottom of the entity is actually after the comment form, so the toolbar would zip down to the bottom of the screen, trying to get to the bottom of the entity if you scroll down far enough. I've hidden the comments with CSS with its entity is being edited. We'll probably want to discuss that decision.
This patch was built on:
81b2685cb6c79cacdd49313f5928d79da84d7f8c
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #163
Bojhan CreditAttribution: Bojhan commented@jesse your last comment is about the UI, is it ready for such review?
Comment #164
jessebeach CreditAttribution: jessebeach commented@Bojhan, yes, ready for UX review!
And maybe I should include the patches this time :/ Derp on me.
You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/edit-tempstore-1901100-48.patch&patch[]=https://drupal.org/files/entity-toolbar-1678002-162.patch
You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.
This patch was built on:
5f67fd1d0858c6f52b4f2685a45b673d6b33e449
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Comment #166
tkoleary CreditAttribution: tkoleary commented@Bojhan
I would hold of on UX reviews on this for a bit there's just too much cleanup that still needs to be done before it even makes sense.
Comment #167
nod_Dumping a JS review:
in ContextualLinkView.js
no?
There are 3 JSHint errors. Forgot a couple of semicolons, cmon :D
EntityToolbarView.js
(that.$fence && that.$fence.css(offsets));
Regular if is better.
make a$body variable.
That's kinda painful to read, any chances you can make intermediate functions to keep the nesting down?
Nothing in particular about EntityModel.js
So yeah, tiny details. No major problems on my end I suspect the todos are too big to solve now, there are follow-ups? Little position problem probably because of some border with or maybe it's by design, don't know, not a big deal.
As far as JS goes feel free to RTBC when tests are green.
Comment #168
Bojhan CreditAttribution: Bojhan commented@tkoleary Thanks for pointing that out, I will probably be able to review next week when I am back.
Comment #169
jessebeach CreditAttribution: jessebeach commented@Bohjan and @tkoleary, we can iterate on the specifics of the UI once the major code changes are in place, those being the changes represented in this issue. I will open followups for any that are missing following the comments in #167. We can have one for UX refinements as well.
Comment #170
jessebeach CreditAttribution: jessebeach commentedyes. that should indeed be
closest()
Cleaned up.
Added.
Pulled the anonymous functions out into named functions scoped to this method.
Regarding the @todos in the code, I removed most of them. Upon further reflection, they really didn't describe any refactoring that is strictly necessary. The couple @todos that are still left over have been given a specific task for followup. The tasks are dependent upon other fixes being committed to Drupal, so they are blocked by dependencies.
#1800022: Update Backbone and Underscore
#2029999: Refactor Drupal.AJAX calls in Quick Edit module JavaScript once #1533366 is committed.
I corrected the failing tests as well. I'm posting this patch to have the bot run tests. tkoleary has a few small styling tweaks that he'd like to apply, so there will be one more patch later today before we can start considering this for RTBC.
Comment #172
Wim Leers#170: entity-toolbar-1678002-170.patch queued for re-testing.
Comment #174
jessebeach CreditAttribution: jessebeach commentedThis patch should fix the failing test.
I've also gone through a visual design review with tkoleary and incorporated his feedback. I'm posting updated images to the issue summary as soon as I post this patch.
Comment #174.0
jessebeach CreditAttribution: jessebeach commentedupdated tests
Comment #175
jessebeach CreditAttribution: jessebeach commentedPosting images for the summary.
Comment #176
Wim LeersValidation of this patch
Testing of this patch
Since this is almost exclusively JS code, and we have no automated JS testing in Drupal 8 core (sadly), and the number of possible scenarios is mindbogglingly large, Jesse went through great efforts in ensuring correctness/preventing breakage:
Conclusion
This patch has received an extraordinary amount of scrutiny and review, to ensure maintainable code and correct functioning.
As I said, I reviewed (and rerolled) the patch at several points in time. However, I did not play a leading role in driving this home. Therefore, I consider myself to be in a position where I can credibly RTBC this.
Note that this must be committed in tandem with #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits — first commit #1901100, then this issue. #1901100 has been ready for many, many weeks, but it only makes sense to commit it together with this patch.
Comment #177
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #178
Wim LeersYay!
But: could you please also commit #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits? As I said in my last comment: that patch and this one must be committed in tandem.
Comment #179
jessebeach CreditAttribution: jessebeach commentedRemoving tags.
Comment #180
Wim LeersComment #181
jessebeach CreditAttribution: jessebeach commentedadding tags.
Comment #182
Wim LeersI think we cross-posted… :P Geez, what a mess did we create!
Comment #183
webchickNot sure if it's strictly related to this issue, but I came across #2037687: Can no longer in-place edit body fields when testing today.
Comment #184
Wim Leers#183: As Gábor replied in that issue: #2035083: Fix body in-place editing in teasers (text editors should trigger formUpdated event) and #2031385: Editor's in-place editing AJAX endpoint broken because of #1043198 and routing system bug fix that.
Comment #186
Bill900 CreditAttribution: Bill900 commentedComment #186.0
Bill900 CreditAttribution: Bill900 commentedupdated screenshot images