Can Picafort
Account created on 11 July 2009, over 14 years ago
#

Merge Requests

More

Recent comments

e0ipso Can Picafort

I am a bit puzzled by this. This code should only execute when sending a response from the storybook.render_story controller. Which in this case is guaranteed to pass those if statements, right?

In any case, I am all in for defensive coding. So I will merge this one.

e0ipso Can Picafort

Thanks for this!

Yes, I agree with @markie here. We should have a section (hidden under a details tag) that retains the previous docs. Something like Alternatively, you can also configure it manually:.

What do you think?

e0ipso Can Picafort

This looks good. Let's give it a go! Thanks!

e0ipso Can Picafort

e0ipso β†’ made their first commit to this issue’s fork.

e0ipso Can Picafort

Add schema for empty props.

e0ipso Can Picafort

@mortona2k that is a cool technique! I would love to know more about it. Would you be up to providing a short screencast explaining it? Or maybe even some explanatory screenshots? I know I am asking a lot from you lately, feel free to tell me so. I appreciate your involvement!

e0ipso Can Picafort

Hi again @mortona2k! It seems like yesterday we were chatting in Pittsburgh.

Component schema is mandatory for integration with SDC Display. The way to declare empty props is not by leaving them out but by actually saying they are empty. Perhaps a schema like this would work:

$schema: https://git.drupalcode.org/project/sdc/-/raw/1.x/src/metadata.schema.json
name: Info Card
props:
  type: object
  properties: {}
slots:
  card_title:
    title: Card title
  card_year:
    title: Card year
  content:
    title: Content

Thanks to this report, I have created an F.A.Q. entry in our documentation. I hope this fixes your issue. https://www.drupal.org/docs/develop/theming-drupal/using-single-director... β†’

I think this is an error message we should work on in core, so it doesn't blow up in contrib. Do you know if you had validation turned on in your local environment? If so, did it show any validation error based on the empty props declaration? I think we should have an issue for Drupal core to provide a good error message for this, to avoid tripping more people. Can you create this issue and shoot me the link in Slack?

Additionally, I think we also need an issue for https://github.com/e0ipso/schema-forms-php to provide a useful error message when validation is turned off for SDC in Drupal. Would you also create that one please? πŸ™Œ

e0ipso Can Picafort

Added new F.A.Q. entry for empty prop schema.

e0ipso Can Picafort

@catch you are correct, that is some cruft from the other day when I split that service in two. I will fix now.

e0ipso Can Picafort

Sorry @longwave it seems the suppression file had a syntax issue. It is fixed now.

e0ipso Can Picafort

@longwave I fixed the library bc layer and added a test for it.

e0ipso Can Picafort

Oh, would also be interested in your thoughts on service naming as well, because all other core services have friendly string names instead of just class names.

This is something suggested by @larowlan in the MR that got SDC in core as experimental.

e0ipso Can Picafort

Throwing something at the CI before logging off. I will finish applying the feedback in the morning.

e0ipso Can Picafort

@longwave thank you so much for your rounds of review. I know this is a big MR, with lots of context, and most of it is hidden behind children issues. I tried to organize it in sub-issues to facilitate development and review, it turns out it's been more difficult for everyone involved. Sorry.

e0ipso Can Picafort

The BC layer for the component validator service is incorrect.

You are correct, I will push a fix right away.

e0ipso Can Picafort

TwigExtension is getting quite large, should the component extension just be a separate service?

Yeah. We touched on that during our conversations while moving it. This is the comment from @plopesc:

Creating a new twig extension would be much simpler, but in the long term, I believe that will be better to have a single one.
I'll work on the BC approach for now.

Do you think that reverting the changes to TwigExtension and moving the SDC Twig extension to Drupal\Core\Template\ComponentsTwigExtension is a better approach? I felt good about both options back in the day, so I'll be happy to implement it.

The relevant conversations can be found in πŸ“Œ Move SDC Twig\TwigExtension to Core namespace Active , and its corresponding MR.

e0ipso Can Picafort

The library prefix - do we need to consider BC?

Yes. This is how we tried to handle it. This section of the code generates the libraries with the old names for backwards compatibility reasons.

I see that there is a small error with the wording of the deprecation message that I will fix right away.

The relevant conversations can be found in πŸ“Œ Move SDC sdc.module functions to Core namespace Fixed and in its corresponding MR.

e0ipso Can Picafort

I don't have any D10 site running with Warmer. This looks like it should fix the issue. I will give this a go.

e0ipso Can Picafort

@longwave most of those comments belong to code that has been moved, rather than changed. This MR is scoped only to moving the code that was approved in the experimental module to its final home. There will be minor changes in code to ensure BC, and some new code in the LibraryDiscoveryParser, but that should be it.

Even if the diff is very big, the changes are not that big.

That being said, I am interested in pursuing the conversations you started but I fear that might derail the scope of this big-enough MR. What do you thing is the best way to address your concerns? Would a follow up issue be a good place for it, or would you rather do it all here?

e0ipso Can Picafort

@longwave are you good to move it back to RTBC?

e0ipso Can Picafort

@longwave I addressed your comments in the MR.

e0ipso Can Picafort

I just re-rolled it. Let's see if that fixes it.

e0ipso Can Picafort

Can you also post the version of Drupal core, the version of the module, the version of e0ipso/twig-storybook, and the version of twig/twig?

e0ipso Can Picafort

@quietone we created a feature branch for this issue. We then opened MRs to this issue's branch to start contributing code in smaller bits. This allowed us to have a more manageable scope for peer review and for comments. All the code accumulated in this branch, as soon as we thought it was ready.

Now this issue's branch contains all the code, it is the big MR against 11.x.

I hope that makes sense.

e0ipso Can Picafort

Updated IS with suggestions from @catch.

e0ipso Can Picafort

I agree with @markie. You should uninstall the addon.

e0ipso Can Picafort

I am thinking that the `name` property for the top level meta, and the `title` for the stories is required by storybook. Do you remember this bit?

e0ipso Can Picafort

Closed in GitHub. I believe there was a missing section in the stories format.

Thanks for taking the time to try this out and for reporting!

e0ipso Can Picafort

I see the suggestions on the MR have not been addressed or contested yet. Setting back to Needs Work.

e0ipso Can Picafort

I think this solution is good. I am not 100% sure either, but I am convinced your solution is appropriate for alpha testing. We can address regressions when/if they arise.

e0ipso Can Picafort

Thank you Pedro for the MR. This was merged now, and I will release it in a couple minutes.

e0ipso Can Picafort

How do you think we should handle this? The main challenge I see with supporting the feature is that we don't necessarily have a top level element in all possible components. Should we enforce a top level HTML element for this? Or should we remove the settings?

e0ipso Can Picafort

I am hesitant to merge this, because this module will soon be removed from core (and merged into the Core namespace instead). This will cause this module to download the contrib module instead.

How do you think we should handle that?

e0ipso Can Picafort

Yay for green tests!

e0ipso Can Picafort

e0ipso β†’ made their first commit to this issue’s fork.

e0ipso Can Picafort

Tests are green now. This is finally ready for review.

e0ipso Can Picafort

I forgot to remove the dependency of Umami on SDC. Let's see if tests pass now.

e0ipso Can Picafort

I also see this error:

Drupal\Core\Extension\InfoParserException: Extension Single Directory Components (core/modules/sdc/sdc.info.yml) has 'lifecycle: deprecated' but is missing a 'lifecycle_link' entry. in Drupal\Core\Extension\InfoParserDynamic->parse() (line 95 of core/lib/Drupal/Core/Extension/InfoParserDynamic.php).
e0ipso Can Picafort

This project is becoming a Drupal module soon. Stay tunned.

e0ipso Can Picafort

This project is becoming a Drupal module soon. Stay tunned.

e0ipso Can Picafort

At @cmlara's suggestion I will create a request in the infrastructure issue queue for the theme => module conversion later on.

I will use this as a reference: https://www.drupal.org/project/infrastructure/issues/3396740 β†’

e0ipso Can Picafort

I confirm I agree to this. I also confirm that I plan to add a module to this namespace.

I don't know the etiquette in this issue queue, but I am RTBC-ing just in case.

e0ipso Can Picafort

I think I got the class_alias wrong. I believe I need to change the alias direction.

e0ipso Can Picafort

I think this will be easier to get rolling as a patch, rather than an MR.

e0ipso Can Picafort

Postponing until we have a 12.x branch.

e0ipso Can Picafort

In principle, I agree with this. However, I am not sure that requiring a default value is a high bar to set. Specially since this is a site building task, that you won't do that often. I am thinking about entities that are created without a form (programmatically, or via JSON:API, etc.). Drupal still does much of the data validation through forms.

e0ipso Can Picafort

I scaffolded https://www.drupal.org/node/3410260 β†’ so we can have a URL to reference in the deprecation messages.

e0ipso Can Picafort

@bnjmnm how do we move this forward? Should we write a document about BC expectations in SDCs inside of Drupal core? Where should that live? I propose a documentation page, child of the official SDC documentation https://www.drupal.org/docs/develop/theming-drupal/using-single-director... β†’ . Any other ideas?

e0ipso Can Picafort

Tests are green and all the initial tasks are done. I think it's time for a review.

Production build https://api.contrib.social 0.61.6-2-g546bc20