Account created on 10 June 2022, over 2 years ago
  • Senior Software Developer at ImageX 
#

Merge Requests

More

Recent comments

🇧🇷Brazil joaopauloc.dev

Hey @pfrenssen, I'll push the changes.

🇧🇷Brazil joaopauloc.dev

I'm attaching the patch with the implementation of @carolpettirossi suggestions.

🇧🇷Brazil joaopauloc.dev

I am attaching the patch with the first proposal.
There are some scenarios for moving registrants from the waitlist that can be tricky to find a good solution.

For example, there are two users on the waitlist. The first one requested three spaces, and the second registrant requested just one. Then, one registrant is removed from the event that updates one space. What should be the behavior in this case?
Should we wait until more space is available for the first registrant?
Or should we find the first registrant on the waitlist that matches the requested spaces with the currently available spaces?

Another point is, that if there is only one space and we have users on the waitlist requesting more than one space, should we block the registration until the waitlist user is promoted?

🇧🇷Brazil joaopauloc.dev

Hey @pfrenssen.
I noticed that the promoteFromWaitlist considers only one registrant to be promoted, the next one.
For this new scenario with seats, I wonder if we could consider changing this behavior.

For example: An event that has 10 seats and there are two registrants in the waitlist, the first one requesting 2 seats(Registrant A) and the second one 1 seat(Registrant B). Let's suppose that someone at an event with 5 seats is removed. Currently, only the first registrant with 2 seats is promoted(Registrant A), even with more spaces in the event for the second registrant in the waitlist (Registrant B).
Then, if another user accesses the page and tries to register the user will be registered, and the second user(Registrant B) on the waitlist is still in the waitlist.

So, what do you think if we update the promoteFromWaitlist function to iterate by the registrant while we have space on the event, instead of getting only the first one?

I did some tests in my local adding the following code in the last line of the function promoteFromWaitlist and worked.

$stillAvailable = $this->retrieveAvailability();
if ($stillAvailable > 0 && !empty($first_waitlist)) {
   $this->promoteFromWaitlist();
}
🇧🇷Brazil joaopauloc.dev

hey @pfrenssen.
I agree with you regarding the route stuff.
I tested your merge request, and it works.
Thanks.

🇧🇷Brazil joaopauloc.dev

Good catch @finnsky. I forgot the once + context stuff, thanks.
Thanks.

🇧🇷Brazil joaopauloc.dev

Hi folks, follow the image below of the preview editable areas.

🇧🇷Brazil joaopauloc.dev

Hi folks.
First of all, great module!!! good job.
This module saves me a lot of time.

With the fixed applied I was able to add the fields as expected.
Take a look at the image below.

🇧🇷Brazil joaopauloc.dev

I didn't push the webform changes. It was a local test. Whatever, I tested it again in a new instance, and worked like a charm.

Thank you @codebymikey. This feature will be available in the next release, 1.0.4. I'm working on other tickets including one of you who opened and we plan to launch everything together.

🇧🇷Brazil joaopauloc.dev

I couldn't reproduce it, it probably was fixed in the new release.

🇧🇷Brazil joaopauloc.dev

hey @codebymikey, thanks for your contribution, appreciate that.
but unfortunately, the changes didn't work in webforms.

I changed the field to_divide to use radios instead numbers but the field was not rendered as expected.

Follow the screenshot.
Field display:

Field settings.

🇧🇷Brazil joaopauloc.dev

joaopauloc.dev changed the visibility of the branch 3464607-radio-support to hidden.

🇧🇷Brazil joaopauloc.dev

I tested the patch and the performance has been improved a lot.

Without the patch the script take 51.7ms to run.

With the patch became 1.4ms.

Nice!

🇧🇷Brazil joaopauloc.dev

Updating the title since the markup doesn't work only with multiple expressions in the markup content.

🇧🇷Brazil joaopauloc.dev

Hi @thachakrit.

Sorry for the late response.

How did you configured the cities fields? can you add a screenshot?
I just saw right now that we didn't add some import information in the help text to configure the params, sorry about that.
You need to specify that you are using the param from the webform adding something like the image below.

Also, I'm facing some issues with cache, so try disabling the cache at /admin/config/system/webform_remote_fields for a moment just to check.

Thanks.

🇧🇷Brazil joaopauloc.dev

Hello @codebymikey, thanks for your contribution.
I'm going to review the module to add the support that you mentioned.
I agree that would be nice to have the support for that.
Thanks.

🇧🇷Brazil joaopauloc.dev

Hi @ty10086, sorry by the any status update, but unfortunately I didn't have time yet.
i'll probably take a look next days.
feel free to text me on the slack, joaopauloc.dev it's my user on slack as well.

🇧🇷Brazil joaopauloc.dev

Hi @igor, despite the tests pass, I think I need more tests to merge this one into the release.
But let us know if patch fix your issue case.
Next days I'll take a look again and write more detailed unit tests to make sure that everything is working as expected.

🇧🇷Brazil joaopauloc.dev

Hi @ty10086
At this moment we don't have support to composable fields, but we will check to support this webform field as well.

🇧🇷Brazil joaopauloc.dev

patch #29 worked for me, Drupal 10.2.6m thanks!

🇧🇷Brazil joaopauloc.dev

Thanks @igorbiki for the quick update.
I could reproduce the issue. I'll take a look.

🇧🇷Brazil joaopauloc.dev

Hi @igorbiki thanks for reporting the issue.
I could reproduce the issue.
I added the unit test for the issue and also i'm adding the patch to fix it.

🇧🇷Brazil joaopauloc.dev

The patch views_load_more-3407928-1.patch worked as expected! Thank you so much @riyas_nr
Check in the image below.

I've created the merge request because the patch didn't work in the first moment and I made a quick fix.
But after spending time reviewing the path I could see that I was using the wrong token(@current_record_count) instead of the @end token.

🇧🇷Brazil joaopauloc.dev

Same issue here, thankfully for the @chejim comment I could fix, this setting that we need to do when we use a different view format could be added on the module page, this documentation will save time from other people.

To reproduce, just create a view and set a format that does not use the .view-content as a wrapper and the issue appears.

Steps to reproduce added on issue summary.

🇧🇷Brazil joaopauloc.dev

Hi folks, i was using the patch and didn't applied after update the module to 2.x version.
I couldn't reproduce this issue anymore on 2.x version.
Taking a look at the code sounds like this issue was fixed by Incorrect integer value for column 'gid'

🇧🇷Brazil joaopauloc.dev

@ckrina the current link on the Tugboat at the moment that I'm accessing is redirecting to another website.

🇧🇷Brazil joaopauloc.dev

It's working for me too.

Try to clear the browser cache since we have CSS files changed.

🇧🇷Brazil joaopauloc.dev

merge requested updated with the latest changes in 1.x branch.

🇧🇷Brazil joaopauloc.dev

Hi folks.
Nice work here, this module will increase a lot the usability of Drupal admin UI.
I found this issue because @rkoller mentioned this one in another issue.
Fixed the typo and works fine. See the image below.

🇧🇷Brazil joaopauloc.dev

joaopauloc.dev made their first commit to this issue’s fork.

🇧🇷Brazil joaopauloc.dev

Hello @dqd.
Exactly, the fields are still required after the submission. On the client side, the required status for both fields is updated and I'm able to submit the form.
But, I got an error message saying that field B is required but shouldn't be as I checked option 1.

I added two patches, one with the test unit with the scenario that I mentioned and the test unit will not pass, and another patch with the fix and you can see the test unit passing.

🇧🇷Brazil joaopauloc.dev

Hello @dqd, my sincere apologies for my last confusing and incomplete comment.
Let me explain better.

I have a website using this module on the 4.0.0-alpha1 version.
Then I tried to upgrade to the next version 4.0.0-alpha2.
After this upgrade one of my content types presented the wrong behavior mentioned in this issue.

I have field A which is a radio box with option 1 and option 2.
Also, I have field B that should be required and visible if option 1 is checked on field A
Then, field C is required and visible if field A has option 2 checked.

The expected behavior stopped working after the upgrade from 4.0.0-alpha1 to 4.0.0-alpha2. So, I tried to update to the latest version 4.0.0-alpha5 but the issue persisted.

After my research, I found this issue. In my last comment when I said I couldn't apply, I meant that I couldn't apply the changes of the merge request 3344587-required-fields-that as patch in the 4.0.0-alpha5 version. I saw other code changes that made the patch fail. So I applied the same change to the 4.0.0-alpha5 version and the behavior to make the field required based on the radio box worked.

Hope I explained better this time. Sorry for the confusion.
Thanks.

🇧🇷Brazil joaopauloc.dev

Got the same issue.
The changes works but I couldn't apply.
Attaching the new patch that I could apply to 4.0.0-alpha5 module version.
Thanks.

🇧🇷Brazil joaopauloc.dev

Also, maybe we need to change the icon when we are dragging which now is different from the icon on the table drag.

🇧🇷Brazil joaopauloc.dev

Hi folks, the last commit was missing the built CSS file.
Now you guys can see the new icon applying the patch again.

But in advance in the image below is the new design.
Personally, I think the old one is better, maybe we also need to change the icon of the cursor which now is different from the icon on the table drag.

🇧🇷Brazil joaopauloc.dev

Are we going to keep the drag icons? I think they demonstrate very well the drag proposal and I think we don't have too many different options.
@nod_ do you have any comments about the changes?

🇧🇷Brazil joaopauloc.dev

patch #264(2784233-264-from-262.patch) worked for me in D 10.2.4.
thanks

🇧🇷Brazil joaopauloc.dev

Fixed and already merged into 1.0.0-rc1.
Thanks @mortona2k.

🇧🇷Brazil joaopauloc.dev

I could confirm that the issue was fixed after apply the patch.

Before the patch:

After the patch applied:

Thanks.

🇧🇷Brazil joaopauloc.dev

I did a quick research for drag icons looking for other options but didn't find any different.

Google Material:

Bootstrap icons:

🇧🇷Brazil joaopauloc.dev

Hello @rkoller.

re #99 A: Both details were fixed.

re #99 B: First and second points fixed. now the text Show filtered and Hide filtered is based on the "button" status.

My next steps:

  • Work in more unit tests to cover all new features added by the issue, it's missing some scenarios
  • Work on the remaining tasks by priority
🇧🇷Brazil joaopauloc.dev

This is not the issue that made the test fail @sijumpk.
I commented the some lines on my last commit that broke the feature but I was trying to identify why some tests are failing.
I already tried this option to resize the window locally without the comment work and didn't work.
But I finally found the issue and will commit the fix to the test.

🇧🇷Brazil joaopauloc.dev

Adding two patches as examples of how we could change the tabledrag settings for the core display the vertical icon and for the field_group display the vertical/horizontal icon.

Field group example.

Core.

🇧🇷Brazil joaopauloc.dev

Hey @nod_, the main problem with this approach it's the Manager form display and Manager fields show the vertical/horizontal icon being displayed but the user can only drag on vertically.
I would like to suggest to change the change EntityDisplaysFormBase to work in a different way than the parent works. Then the icon will be properly.
Also, we could create an issue(feature request I think) on the field group module to change the EntityDisplaysFormBase to work as they need.
What do you think?

🇧🇷Brazil joaopauloc.dev

Hello @nod_.

Regarding your comment #36. I was looking for some way to do that.
I couldn't identify a way to make this dynamically for tables that we have h/v dragging. In the case comment on Field form settings, I tried to use other tabledrag settings but wasn't enough.

I checked out the class tabledrag-leaf that makes the drag h/v work added by the form EntityDisplayFormBase. Since this setting is added on the base form, I couldn't identify a way to make this work without any change by the contrib modules like field_group.

I will spend more time this weekend on this issue but I guess without any change on the contribe module will not be possible.

Another possibility could be to keep the form display with the h/v drag icon. As you mentioned in your comment #36 the settings are hierarchical.

🇧🇷Brazil joaopauloc.dev

Thanks for the suggestions @nod_. I didn't know about the indentEnabled setting, much better.
I removed the old setting of the forms.
I also made a list of all the places where we already noticed that the icon should be changed.

Without user configuration.
admin/structure/block
admin/config/content/formats
admin/config/content/formats/manage/basic_html?destination=/admin/config/content/formats
admin/config/media/image-styles/manage/large?destination=/admin/config/media/image-styles
admin/config/regional/language/detection
admin/config/regional/language
admin/config/search/pages
admin/config/user-interface/shortcut/manage/default/customize
admin/structure/taxonomy
admin/people/roles

Needs quick settings.

Field settings default values.
Create a field of the ListItem type add more than one default value and save, you should able to order the default values.

Views
Go to any views and try these.
rearrange fields.
rearrange filter in views.

Workflow
Enable workflow and content moderation modules and go to
/admin/config/workflow/workflows/manage/editorial?destination=/admin/config/workflow/workflows

Regarding the manager form display and manager display I'm working on that.

🇧🇷Brazil joaopauloc.dev

Fixed both pages.
I think we need to add a change record since we changed the table drag plugin to support both icons.
For other modules or even in a core feature to use the vertical icon just need to add the following attribute on the table structure like the example below.

    '#type' => 'table',
    // For filter.admin.js
    '#attributes' => [
      'id' => 'filter-order',
      'data-drag-orientation' => 'drag-y',
    ],

By default, the table drag will use the horizontal/vertical drag icon.
But we need to update the documentation too. But I don't know how to do that.

🇧🇷Brazil joaopauloc.dev

Hello @rkoller and @ckrina.
I have one question about the new design of the input filter and go-to elements.
We also have the case when the user types something that there is no result and we display a message like

There are no blocks matching the filter conditions.

Should we move this message to the left as the link to go to the element was? like the image below.

🇧🇷Brazil joaopauloc.dev

Good catch @rkoller.
In my opinion, we could follow it as a new follow-up issue and just set it as a related issue. Then we can move forward with this one.
Also, as you mentioned is not related to the changes of this issue. I think the changes in this issue make evident the issue that you found.

Production build 0.71.5 2024