Those changes make sense to me. I'll see if I can find some time to take a swing at this in the next several weeks and report back with any questions.
I heard back from the editorial team:
"...a large part of the appeal of workspaces for us is launching/closing a competition, where we have to make many edits to many nodes around the site. Currently that's done manually on individual nodes in succession. With this we can do it all at once and be able to identify exactly what was done where all at once/in one view."
That's a great question. I forget all of the context around the original request from the editorial staff, but I believe it was centered around the idea of not wanting to have to click that button for each node when a workspace might contain changes from dozens of entities. Let me see if someone still on the project can chime in here with more information.
I ran into the same issue and it stumped me as I'm not a front-end developer. Another developer on my team pointed out that Drupal 11 ships with jQuery 4, Select2 doesn't seem to support jQuery 4 yet, and the isArray method is deprecated in jQuery 4, hence the issue.
Here are some links that provide more context:
So we decided to go with the Tagify → module/library until this is resolved.
Hope that helps!
adamzimmermann → created an issue.
Are you saying that the chosen module offers that now?
Nope, that's what led me to this module!
heddn → credited adamzimmermann → .
I can confirm this issue exists. I was hoping to switch to this module from Chosen, but this is a blocker. I'm loving everything else about this module though!
I have a commit ready to add the tests, but I don't seem to have access to the MR.
I'm not sure we need a test for the issue reported in #12 since the new approach doesn't have that issue, and from my experience of trying to do it the wrong way too (before I found this issue), the fact that we can load the node edit page earlier in the test proves there isn't an issue.
I upload a patch if that helps.
I'm using 2.0.0-beta3 and the changes in MR 64 work perfectly for me and solved a client request. Thank you!
So RTBC from that perspective, but I see that this MR targets the 8.x branch, so I'll refrain from updating the issue Status for now.
adamzimmermann → created an issue.
Should this be marked as "Fixed" now?
The issue referenced in the previous comment was merged. Re-rolled the patch so it applies to 2.1.2
If you are using the diff module with the Workspaces Extra module → , you will need the patch I just uploaded to alter the service definition.
adamzimmermann → created an issue.
This seems to have fixed the issue for me too.
This takes the work from the patch in comment #3 and adds return types to fix a WSOD issue and removes some debugging code I found in the patch. I'm able to load the diff route now with this module enabled.
FWIW I based my patch branch off of the 2.0.x branch and the patch in #3 applied cleanly as my starting point for updating it.
This works in my local testing, now to see if it works for others.
Should this issue be for version 2.0.x-dev? I think that is why the MR I opened had a diff wildly different than what I expected.
Going to try take two on generating a new patch that adds return types to the methods to make them match the method signatures in the latest diff module.
adamzimmermann → made their first commit to this issue’s fork.
adamzimmermann → created an issue.
For anyone else encountering this, the Workspaces Extra module → provides a much cleaner interface that "solved" this issue for us.
The MR in its current state fixed the issue I was having. However, I wasn't seeing this in relation to CKEditor. I was having this issue after enabling the core Workspaces module and some associated modules. I can provide more detail if that would be helpful.
For anyone else having this issue, the Workspaces Extra module → seems to help solve this and many other issues. It feels weird to need a contrib module for core functionality to work though.
Assumption: Most Drupal sites are uploading files using media entities, and media entities throw the error noted above when trying to edit them in the non-default workspace. This makes the workspaces module unusable (with media entities) as someone trying to implement it on a new site build right now.
I don't see any recent activity on this issue and the last patch seems to have issues from what others are reporting.
Is the workspaces module simply not being used on production sites yet, or how are others getting around this?
Do we need to alter media entities like they have done for files in 🐛 Cannot create entity with image in a workspace Fixed ? Are there known side-effects to this that make this not a viable solution?
Any insight is much appreciated!
apotek → credited adamzimmermann → .
I updated the MR so it applies to the latest branch cleanly. I regenerated the CSS, but front-end development is not my strong suite, so hopefully I did that correctly.
However, I'm unable to apply the patch due to it containing binary file changes. I got around that by simply removing those changes in my local patch file, but then I'm missing some icons. How are others testing this?
A few notes:
I'm still seeing the issue reported in #29 (likely due to my patching issue).
Style to match Gin branding.
This is listed in the issue description, and I think it would be great to try to do this still. If someone can point me in the right direction on the approach we want to take, I can take a swing at it.
While the MR in its current state is a big improvement. The interface is still very different from anything else rendered in the Gin theme. This seems to be due to the CSS from the core module. Perhaps we need to be more aggressive in overriding it to provide a cohesive admin experience?
Toolbar with the current state of the MR:
adamzimmermann → made their first commit to this issue’s fork.
adamzimmermann → made their first commit to this issue’s fork.
adamzimmermann → made their first commit to this issue’s fork.
I'm not sure what is going on with the preview-image-styles fork, but the patch from that MR would not cleanly apply. I suspect it has something to do with the fork being out of sync. I tried to update the fork, but there was a conflict.
I didn't have the time to fully sort this all out, so I just recreated the patch for the 2.x branch.
I tried to create a new MR instead of a patch (hence the new extra branch), but again I was having issues with the fork being out of date and the 2.x branch in the fork was not up to date enough to create a patch against.
Hopefully this patch helps someone who can sort this out, or just apply the patch directly like the old days.
Unfortunately the patch also includes a fix from another issue, as trying to apply both patches at the same time was causing a patch application failure. However, both patches were needed, as the other patch allows the preview controller to not throw an error ( 🐛 Preview results in Error: Call to a member function getDefinitions() on null Active ).
Once that issue is merged, the patch will need to be re-rolled. Until then, I can confirm that this patch works on 2.x.
Perhaps the "Version" on this issue should be updated to 2.x-dev as well? I'll let the powers to be handle that though.
adamzimmermann → made their first commit to this issue’s fork.
The patch supplied in this issue works.
I can also confirm that this fixes the issue.
This seems to duplicate the work in 🐛 imageEffectManage not assigned RTBC . This MR includes some additional refactoring though.
I'm working on a new project where they have built their own version of SDC with a slightly different feature set, but a similar ideology. When I saw it I thought of this issue and wanted to share the approach.
We are integrating with generic Twig templates from a component library, and needed a way to map data to specific variables with specific formatting/values in a way that kept that mapping connected to the Twig template it was used with.
The use of a custom plugin system was the solution they architected. Each component/Twig template has an associated plugin class that does the mapping, but it could be used for preprocessing too.
I agree with @CtrlADel here. We should not have unrelated files change how the component renders, we'd be violating the Single in SDC.
I would second this, and it seems that I'm sharing something similar to the plugin idea shared above in #3321203-18: Have a way to implement the a preprocess function per each SDC component (ideally in the same folder) → .
Hope this helps.
adamzimmermann → created an issue.
apotek → credited adamzimmermann → .
Excited to see what you come with here. I'm not surprised there are some inconsistencies and issues with the refactoring to where queueing occurs mid-project.
I would like to push this forward if possible still. @apotek what are your thoughts on some of the issues you raised?
Thank you for creating this follow-up issue.
@apotek if you have time, feel free to work on this too, just let me know so we don't duplicate effort.
trim() requires a string, but $row->get(configuration['field']) returns a field
What is a "field"? I'm trying to work on a fix, but genuinely unsure what the code change should be to prevent the issue.
This conversation shows the history of this change.
https://git.drupalcode.org/project/orange_dam/-/merge_requests/43#note_2...
adamzimmermann → made their first commit to this issue’s fork.
I just ran into this issue and confirm that it causes a WSOD.
I can also confirm that the patch solves the issue.
I haven't done extensive regression testing, but my initial testing of our endpoints shows no issues.
The initial MR was merged, but I would like to keep this open for adding the final polish/fixes as the work I did was more of a prototype of the functionality and not feature complete.
@apotek has noted some of those changes in his comments above.
I think, given the option of having to re-queue 1 million items from the start, if the process is interrupted, versus possibly having some paging overlap or possible interstitial changes (which could be queued later with a date-based query), I would prefer having to re-queue and re-migrate a few hundred over having to requeue and remigrate a million. I think even a "sloppy" resume is worth it. My $0.02
When you put it that way, it makes my concern seem like less of an issue haha.
I have a MR coming. I don't think it's ready for merging or final review, but it could be a interim solution that we use via patch and continue to improve on as time permits. I'm open to feedback on the approach here.
Thank you for creating this issue and providing such a detailed request.
I want to acknowledge that I 100% see the desire for this and the value, but I have two concerns off-hand.
- The complexity around managing the state of the current page/the page that should be resumed from and keeping that separate for different types of requests. This is solvable in my mind though.
- The issue around the result set changing between the initial request and the next request, which might completely alter the items in a given page of results depending on the sorting being used. This is potentially solvable if new items are only ever added to later pages, but that is something we need to determine based upon the sorting algorithm being used.
We will have to think about this more when we get to this.
adamzimmermann → created an issue.
adamzimmermann → created an issue.
@pratik_kamble thank you! This just removed the need for a patch from a PR I had in progress.
The screenshots are compelling. +++
adamzimmermann → created an issue.
I went with this approach:
or keep it as is and create a new function called retrieveContentItem() :p that we call from within getContentItem()?
I named it slightly different though.
adamzimmermann → created an issue.
I like this idea+++
adamzimmermann → created an issue.
Currently, orange_dam drush commands specify the migration with an `--migration` option. But the migrate module specifies it as a bare argument. IOW, we do `drush orange-dam:migration-run --migration=` but the migrate module does it this way `drush migrate:import `
@apotek I actually tried to go down that road and if we go that route, we will lose the ability to not pass a migration and have the interaction choice tool. So I can see both sides of the argument. What are your thoughts?
Cannot add a required argument after an optional one.
I'm going to merge the changes we have so far, and we can continue the discussion about this.
I'm going to take a pass at understanding the scope/feasibility of this.
@apotek I believe the changes I made address the needs you surfaced. It should pass through any options that are sent to it.
I have solved the issues and this is ready for review.
adamzimmermann → created an issue.
adamzimmermann → created an issue.