gargsuchi → credited ameymudras → .
@joshuasosa thanks for the patch. I checked the code and looks like we are fixing the symptom rather than the root cause. The redirect URL is a required field and hence its absolute URL shouldn't be empty in the first place. It would be helpful if you could provide exact testing steps here
ameymudras → made their first commit to this issue’s fork.
@deepakkm while merging i guess you have reverted the changes made by Sumit. Please be careful of the changes. Ive added back the changes
ameymudras → made their first commit to this issue’s fork.
ameymudras → made their first commit to this issue’s fork.
fubarhouse → credited ameymudras → .
larowlan → credited ameymudras → .
gargsuchi → credited ameymudras → .
The patch #4 doesn't apply to the latest 2.x branch. I have re rolled the patch to fix the issue
Apologies, i submitted a wrong patch. I wanted to submit a patch just to remove composer dependency which was preventing contrib variation cache to be removed from the project. Il be more careful henceforth. thanks
@kristiaanvandeneynde even though this module is compatible with the core variation cache the composer.json has a dependency "drupal/variationcache": "^1.0" which prevents removing the contrib variation cache module. I have submitted a small patch to remove the dependency.
Adding a patch here from the MR provided above just so anyone can use in their builds.
Encountered this issue once more and proceeded to upgrade the module to version 2.0-rc5. However, the problem persists despite the update. After reviewing the modifications, it appears that resolving the issue requires either re-saving the settings form or altering the configuration from max_filesize: null to max_filesize: ''.
For an existing installation undergoing an upgrade, this scenario applies. However, for a new installation, it functions seamlessly without encountering any issues.
gargsuchi → credited ameymudras → .
gargsuchi → credited ameymudras → .
Tested #15 and it works absolutely as expected. Only thing that can be an improvement here is using an else if condition rather than if
I tested the solution again on 11.x and I am not able to replicate the issue mentioned in #48. Please find the screenshot attached
I can confirm that the patch #37 still applies to 11.x and works as expected. Re rolling to a MR was not needed here. We might need a supporting test here.
ameymudras → made their first commit to this issue’s fork.
@kim.pepper thanks for the suggestion. Ive added a validation constraint to check for an empty file. Appreciate if you could review and suggest changes / issues if any.
It still Needs tests though
ameymudras → made their first commit to this issue’s fork.
@shweta__sharma My understanding of #57 implementation was to move all the styles related node preview to a separate css file alltogether
Do we really need to fix this issue? user.role.authenticated.yml should always be system generated as opposed to making manual changes.
Raised a MR which removes remaining instances of the libraries that were using node.module.css
ameymudras → made their first commit to this issue’s fork.
ameymudras → made their first commit to this issue’s fork.
Was able to reproduce the issue on D11 as well following the steps provided above. I have created a MR with the patch to fix
ameymudras → made their first commit to this issue’s fork.
I see that the patch provided fixes the issue but any specific reason the patch uses px rather than a standard rem?
Implemented a solution as per #57
ameymudras → made their first commit to this issue’s fork.
ameymudras → made their first commit to this issue’s fork.
ameymudras → made their first commit to this issue’s fork.
ameymudras → made their first commit to this issue’s fork.
Added dependency injection, also as per #13 added a log on unblock but need tests
Couldn't generate the interdiff, changes to fix the issue with above patch and compatibility
ameymudras → created an issue.
gargsuchi → credited ameymudras → .
gargsuchi → credited ameymudras → .
gargsuchi → credited ameymudras → .
gargsuchi → credited ameymudras → .
gargsuchi → credited ameymudras → .
Fixing the issues with the above patch
Rerolling for 10.x, couldn't generate an inderdiff
Fixing the test failure with above patch. But we still need to address #20 completely
ameymudras → made their first commit to this issue’s fork.
Replacing a few more instances of static::class
Would be really helpful if we can have testing steps in the description. Couple of suggestions after code review:
1. we can make use of isset instead of array_key_exists since its faster
if (is_array($text) && isset($text['#markup'])) {
2. The following comment needs to be better worded
// Handle strings that are passed in array as value in the #markup key.
ameymudras → made their first commit to this issue’s fork.
Re rolled the patch #167, the patch doesn't apply anymore
Rerolled the patch
Ive tested the patch on 10.1.x and following are my findings
1. The Issues summary is clear and explains the problem
2. Was able to replicate the problem
3. The patch applies cleanly
4. The patch fixes the issue and eliminates the extra space.
5. Did a code review and its a simple change to offload the locale.admin.css css as per #10
Moving the issue to RTBC, skipping before / after screenshots, since a few have been already provided
@Andy-blum thanks for the suggestions, I've changed the style attribute usage as per your suggestion. Im not sure why we need library-override because we are not extending / overriding any external library and the js/navigation-utils.js: {} that does the toogle function is a part of the global styling. Let me know your thoughts
inset-block-start causes the issue, I have posted a patch which fixes the issue but I will try to come up with a better solution for this issue
gargsuchi → credited ameymudras → .
Attempting to fix the issues mentioned in #33
Tested on 10.1.x
The issue summary is clear and steps have been provided
Was able to reproduce the issue
Used the MR to fix the issue
Did a code review and no issues were identified
Screenshots have been provided above
I just had a look at #33 and I'm not sure why we are using
+use Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem;
Trying to fix the test failures
ameymudras → made their first commit to this issue’s fork.
Kristen Pol → credited ameymudras → .
Kristen Pol → credited ameymudras → .
Kristen Pol → credited ameymudras → .
Kristen Pol → credited ameymudras → .
Removing deprecated code from patch #10 and making it compatible with 10.1.x
Tested on 10.1.x and following is my observation
1. The issue summary is clear and explains the overall problem
2. Testing steps have been provided and was able to reproduce the issue
3. Patch #24 applies cleanly and the non-ASCII config entity sort correctly
4. Tests have been included and passes for #24
5. Did code review and no issues were identified
Marking this as RTBC, not including additional screenshots. Already provided in #27
Did a brief code review and the following comment doesn't make sense to me, I think we should reword it. Otherwise, the code looks good
// We might use the passed entity instead of the unchanged one.
Tested on 9.5.x patch #24
1. The issue summary is clear and explains the problem
2. The code change is as expected and as described in the issue summary
3. Applied the patch and checked for the "back to content editing" link on the node preview, which is shown as expected
4. Additional tests are not required in this case
Looks RTBC to me.
ameymudras → made their first commit to this issue’s fork.
Fixing the CCF issues with #69
Tested on 10.1.x and this seems to be a good improvement. Issue summary is clear and screenshots have been provided above. Im marking this as RTBC