Oops. You asked me to go ahead and merge. Done. 🙂
Looks good!
Congratulations on your first Core commit, @secretsayan! 😄
Approved!
wim leers → credited traviscarden → .
traviscarden → created an issue.
traviscarden → created an issue.
traviscarden → created an issue.
traviscarden → created an issue.
traviscarden → created an issue.
traviscarden → created an issue.
Thanks, @jessebaker; this is huge. 😉
traviscarden → created an issue.
Nice. 🙂
traviscarden → created an issue.
traviscarden → created an issue.
I've marked 🐛 Canvas flickers when adding a component to an empty slot Active as a duplicate of this issue and added the screenshot and additional detail from it to this issue's summary. I don't know if they have the same root cause. If they turn out not to and can't be easily solved at the same time, we can re-open that issue.
That makes sense, @lauriii. I could certainly imagining the root cause being the same. I'll close this one as a duplicate and add the added detail to that one. Thanks.
@secretsayan asked me to push this change for him real quick because he's in the process of moving to a new laptop.
Thanks, @wim leers. I agree with your assessment. I think the original impetus of this issue was based on a misunderstanding, and the value to be derived now is just findability in the short term. I've updated the summary accordingly.
Also, like you, I assumed that adding the local task would be a matter of minutes, but I must be missing something. I can get a local task to appear if I hard-code the node ID, but I can't get it dynamically. Anything I try results in either a fatal error or just not showing the task. I'll create an MR with the hard-coded nid. I need someone to help me figure out the last part.
traviscarden → created an issue.
traviscarden → created an issue.
Thanks @omkar-pd; that fixes the presenting problem, but apparently it was supposed to be red.
I think that's intrinsic to the scope of this issue. Nodes are the presenting question (and the answer seems obvious to me). I assume most other entity types already have their own conventions--Views, blocks, media, etc. Maybe we should break this issue up a little bit: 1) keep this one but scope it at Nodes only, 2) create one for deciding on other entity types when we're getting ready to implement them, and 3) expose the ability the define the behavior in the API. What do you think?
traviscarden → created an issue.
traviscarden → created an issue.
Lol. "felxibility" 😅
traviscarden → created an issue.
wim leers → credited traviscarden → .
Nice! I prettified openapi.yml and approved it.
Re: #29
🟢 Scenario 5.1: Deleting created entities when abandoning progress
There should be a way to delete the draft by using a delete action but once the user has entered the content form and inserted any content there, we should retain the saved draft.
I don't understand. I assume "saved draft" means an auto-save state. Is that right? What do you mean by "content form"?
🔵 Scenario 6.1: Deleting created entities when abandoning progress
There might be different expectation between [Media entities and taxonomy terms]. [In] the use case of Taxonomy Term because… you're just adding a tag inline using an autocomplete.
But you can also configure Taxonomy term fields to "create referenced entities if they don't already exist". The point is that it’s possible to create separate entities from a node edit screen. Does the rest of the scenario look right?
🟡 Auto-save only applies to saved entities. It doesn't make sense in any other context, e.g., an un-submitted node/add/* form, since (it is also assumed) Experience Builder can't even be opened except on an existing entity.
I don't think this is the right assumption. Content types that are using Experience Builder should open in XB even when creating content.
Okay, so when you create an XB-enabled node, for example, there is no standard node/add form at all? You go straight to Experience Builder and that's the only way to interact with the entity? If so, that should be added to the assumptions. And we probably need a new scenario for editing the overall entity information--such as title, URL alias, and menu settings--from within Entity Builder. Would you agree?
🟡 If a user wants to revert back to a specific point in history, that will be handled with revisions.
It should be possible to revert back to previous revisions and auto-save states. It would be okay if auto-save states get truncated into a single revision once content gets published.
Wouldn't that mean that the "truncated" (meaning, squashed or merged, I assume) auto-save states are always identical to the list of revisions--making them effectively useless? Wouldn't that mean that every revision would have an identical auto-save state?
🟡 What happens if the same user has the same node open in Experience Builder at the same time on two different computers? For example, what if a user goes back and forth between home and the office? Does one computer get locked out of editing and become "read only"? How do we avoid the two computers repeatedly overwriting each other's auto-saves?
We could do something similar to: https://www.youtube.com/watch?v=CsRBypaT_hM&t=27s. We could apply this both for the scenarios where the user is the same and where it's different.
Given the workflow depicted in the video, I suppose that would mean a user on computer two would have to request control from computer one. And ignoring the logical challenges with that, it would mean that our auto-save states would no longer be associated with a user but rather with a browser session, right? Otherwise the concept of locking doesn't make any sense and we haven't solved the problem in question at all.
🟡 What happens to one user's auto-save states if a node or revision is changed by another user?
Now that I understand that only one user can be working on a single entity at a time, problem can't actually materialize, then as long as starting an editing session in Experience Builder locks the whole entity, this is irrelevant.
"Wounded." Haha. No, it was actually dead. As I understand @tedbow per a conversation we had outside this issue, it was removed on purpose and put back in by accident during a merge or rebase. Trust us, @wim leers : we don't make mistakes around here. At least not when we're correcting our mistakes. 😉
A lot of the details in this issue (that assumption included) are pending confirmation from @lauriii. See the legend for interpreting the status markers.
Granting our assumptions, it seems like we're more or less in agreement on the proposed resolution at this point. (Tell me if I'm wrong.) I'll see if I can get @lauriii's eyes on the issue, and in the meantime, I'll find a good place in the codebase to document our architectural direction. Since the scope of this issue is research, that should resolve it, and we can create follow-ups to start on implementation.
The wording of these user stories assumes that the entity has already been explicitly saved (i.e. exists as an entity). autosave_form also handles the case where nothing has been explicitly saved ... I don't know if that is an XB requirement but it would be good to explicitly rule it in or out, currently it's not mentioned.
Good thinking, @catch. I assume it is not a requirement since, as far as I know, there's no way to use Experience Builder except on a piece of content that already exists.
I've updated the issue summary to include this assumption.
Is that a question, @wim leers? 🙂
Since 🐛 Openapi.yml uses patternProperties which is not supported by our dependencies Needs work is yours, @tedbow, I'll ask you to approve this.
traviscarden → created an issue.
CRDTs = Conflict-free replicated data types. Some interesting discussion on Drupal Slack.
Okay, my updates to the issue summary are complete. I think I've captured more-or-less everything. Assigning to @lauriii for input on the unconfirmed requirement details.
@lauriii, we've documented our current understanding of the requirements in the issue summary. We would like your confirmation so we can confidently finalize our architectural decisions. Every detail we've identified is marked with a colored circle indicating its status (confirmed or otherwise). (See the legend.) Basically, anything that isn't green needs confirmation--mostly from you.
Incidentally, the issue summary is now rather massive, and I could see it being extracted to its own issue for reference and discussion, if desired. I'll happily do that if asked.
I think this was only waiting on my weigh-in. Approved. 🙂 I don't know who should merge it.
gábor hojtsy → credited traviscarden → .
...and again to updated the proposed resolution. I'll leave it assigned to me to continue fleshing out the distinct user stories for drafts and publishing.
Updating the issue summary to reflect the elimination of Content Moderation as an option per #28.
Oops. I omitted the commit credit on the merge commit. Reverting and redoing correctly. (I don't know why that appears in reverse above, but it is correct now.) And now...
👏 Fixed!
Okay, I've fleshed out the requirements for the Auto-save feature, and this is on track to become the longest issue summary in history. 😬 (I blame d.o's CSS for the odd markup choices I made to make it more readable.) I plan to do the same for each of the other two features and then distill the discussion in the comments. Also, it's past 1:00am my time and too late for proofreading. Here's hoping for the best. 😛
I'm going to try to process all the details here and update the issue summary.
Oh, interesting. I haven't done this before either, but it looks right based on the documentation you pointed to. Fixed.
tedbow → credited traviscarden → .
Nice! Merged. Good work, guys; this is exciting. 🙂
traviscarden → made their first commit to this issue’s fork.
I've been trying to get access to merge this MR all day, and despite apparently having all the necessary permissions, I still cannot. So if someone who can merge it kindly would, that would be great. You have my approval to do so.
Nice. This should immediately improve the experience for everyone.
I don't seem to have the ability to actually merge the MR, so I'll mark this RTBC until I can get it.
I can RTBC this as soon as the (trivial) requested change is made.
Re: #66
4. Blocker: Evaluate dev dependencies · Issue #78 · php-tuf/composer-stager
For this issue, the release managers think that most if not all of the remaining dev dependencies could also be removed. The original issue summary is out of date (e.g., Infection has already been removed). A contributor could help by updating this issue to reflect the current status, so that release and framework managers can make final decisions about which dev dependencies to remove.
I've updated the issue summary there.
traviscarden → created an issue.
I assume this is related: 🐛 String props that are integer values aren't treated as strings Postponed
Yes. That issue has been marked as a duplicate of this one.
I've manually tested again and confirmed that, of course, it still works. A quick review of the JS code (not my area of expertise) should wrap this up.
traviscarden → created an issue.
Thanks, @phenaproxima! Just to document the fact: you and I had a conversation offline in which I invited you to make this proposal.
I like this a lot at the conceptual level. I'm not equipped to weigh in on every detail of the proposed implementation yet. Can you comment on how your proposal might fit into 🌱 [META] Roadmap Active ?
Note: I've created a 3.0.x branch for this.
Actually, the fieldable use case may actually be out of scope for that issue. Let's call it a dependency instead.
Closing this in favor of 🌱 Create a new architecture for checklists as config entities, with tasks fulfilled by conditions Active .
I pushed some code that seems to work locally. I can't run Cypress tests right now for some reason; we'll see what happens on CI. The code probably isn't very elegant, but red => green => refactor, you know. 🙂
Update: I'm on track with a solution, but there's an ugly merge conflict with 0.x in xb-general.cy.js that I'm trying to rebase away.
Thanks, @bnjmnm. That fixes the bug in question in my local testing. Moving to "Needs work" though, since the tests are failing.
traviscarden → created an issue.
traviscarden → created an issue.
This was actually proposed upstream a long time ago in Checklist API (where such a request should be addressed): #2137379: Make checkbox a radio button with additional option(s) → . That issue stalled out, but I think it's a good idea and I'm happy for you to re-open it. There's some good analysis for you to build upon. Marking this issue Postponed to signify an upstream blocker.
I agree: "Getting Started" and "Be Efficient" are superfluous, the UI is basically self-explanatory, and there are better ways to help novice users. At some point, it might be good to add a simple hook_help() implementation. For now, I'm happy with this.
Yes, I think we should deprecate seo_checklist_optional_modules. It think it was mostly created for convenience for evaluators and for local development. I would never recommend using it in production. I think recipes present some much more interesting possibilities.
If anyone is using the module in production, we can't remove it without potentially breaking their site. And we don't want to move the dependencies to our main composer.json, which would exacerbate the problem in several respects. I think it would be best to remove it in a 6.x branch where we can introduce a lot of the other major changes that seem to be coming.
If the version constraints in the module pose a problem in the meantime, we can loosen or even eliminate them (e.g., change ^2 to >=2.0 or even *).
Leaving this assigned to @soaratul, but I'm going to try to pick up where @tedbow left off with the backend part of the work.
Looks like it's almost there. We've just got a PHP warning to fix:
Warning: Undefined array key "render element" in Drupal\Core\Theme\ThemeManager->render() (line 202 of core/lib/Drupal/Core/Theme/ThemeManager.php).FYI: I'm setting this issue aside for the time being in favor of higher priorities.
tl;dr: I agree with everything you said, and I have an "inexpensive" solution I would like to implement that I would consider to satisfy the requirements of this issue.
IMHO functional tests (
BrowserTestBase+ Cypress E2E tests) is sufficient for now — that's sufficient for testing the common paths.
I agree.
The goal for OpenAPI is to help the front and back ends to stay in sync with more ease, not for that to become a goal unto itself.
Yes. We're on the same page here.
That being said, if you see a low-effort/maintenance, high-impact/reach way to write kernel or unit tests to verify certain edge cases in request/response bodies are caught automatically by OpenAPI, then I'd definitely welcome that :)
I have proposed one in the above MR. I actually hadn't planned on looking at this issue at all yet, but working on 📌 Add missing API paths + request bodies to `openapi.yml` Active I was having a hard time testing my assumptions about how the specification itself was being interpreted. And the ability to just whip up a quick PHP array and get instant feedback really clarifies things and saves a lot of "throwing things at the wall" and "testing by coincidence". (Turns out even YAML can be written test-first! 😄)
But that should not be a multi-week/month endeavor IMHO.
It just took a day. 😛
Re: the MR
The tests are passing, but I notice PHPUnit reports two tests skipped--which I assume are the two that this MR adds. That is doubtless because the tests depend on league/openapi-psr7-validator, which I assume isn't installed on CI. Ordinarily that would constitute a problem that needs solving, of course, but I'm actually fine with it in this case, since its whole point is to aid in rapid development and the real "regression tests" are in Cypress. Developers can just run them ad hoc in PhpStorm, for example (as I've been doing), when actually modifying openapi.yml and ignore them the rest of the time. They could technically get out of sync, but as far as I'm concerned, we can create a follow-up to worry about that later.
So I would like to propose the MR above as a rapid development tool that satisfies our minimal requirements and lays a foundation of future iteration if that ever seems valuable. According to your judgment, we can create a follow-up issue and/or a @todo or a note in the README I added to capture the above issue of the tests not getting run on CI.
I think it makes sense to do that as part of this issue?
Yes, that's what this issue is for. I just copy/pasted the wrong issue link in the other comment.
Oops. That was just a copy/paste error on my part. 📌 Add missing API paths + request bodies to `openapi.yml` Active is, of course, the one I meant.
traviscarden → created an issue.
Can you create a follow-up issue to document the routes that do not yet have request bodies documented in
openapi.yml? 🙏 (i.e. for the@todothat @tedbow commented on over at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...)
I believe you're looking for 🐛 OpenAPI validation errors must be provided as a JSON response RTBC .
I've also created 📌 Lay out a strategy for OpenAPI integration Active and 📌 Add missing API paths + request bodies to `openapi.yml` Active . I think that wraps up this issue.
traviscarden → created an issue.