Problem/Motivation

During Beta development (July - November), we have introduced some compatibility breaks with some temporary compatibility layers to ease the transition:

  • ui_patterns_devel was moved to sdc_devel but the module still exist (empty) in the codebase >> remove the module
  • the stories were moved to there own files but the stories from the compoennt definition are still merged with the new ones >> remove the merge logic
  • component() Twig function wax replaced by include() Twig function but >> remove component() Twig function
  • What else?

Proposed resolution

Remove those temporary compatibility layers.

Issue fork ui_patterns-3490872

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

christian.wiedemann’s picture

christian.wiedemann commented
Assigned: Unassigned » christian.wiedemann

christian.wiedemann opened merge request !295

christian.wiedemann’s picture

christian.wiedemann commented
Assigned: christian.wiedemann » pdureau
Status: Active » Needs review
pdureau’s picture

pdureau commented
Assigned: pdureau » christian.wiedemann
Status: Needs review » Needs work

2 feedbacks

Remove the merge logic from stories

You removed the call to ::mergeStories() without replacing it by StoryPluginManager::getComponentStories() :

-    // ::mergeStories() is a temporary compatibility layer.
-    // @todo Replace by StoryPluginManager::getComponentStories() before 2.0.0
-    // release.
-    $component = $this->storyPluginManager->mergeStories($component['id'], $component);

So, I am afraid it is not working well. Did you test by looking the component library?

What else?

Do you know any other similar compatibility layers to remove? I am not sure I was thinking about everything while authoring this ticket.

christian.wiedemann’s picture

christian.wiedemann commented

ah sorry, missunderstood the comment. I just opend the library but there are no stories inside the ui patterns in any test module. A test module would be great and some render tests.

Here is my list what we should change:

  • Move this hook function ui_patterns_field_config_delete(FieldConfigInterface $field_config): void TO ::UiPatternsEntitySchemaSubscriber
  • Remove ui_patterns_plugin_filter_block__ui_patterns_alter
  • Remove ui_patterns_test_theme. I think we don't need it anymore
  • Remove test components alert, close_button, prop_types_tests
  • Remove ui-patterns-actions.html.twig
  • Not sure about ui_patterns_legacy.
  • Not sure about ui_patterns_blocks.
  • Not sure about ui_patterns_field_formatters.
christian.wiedemann’s picture

christian.wiedemann commented
Status: Needs work » Needs review
christian.wiedemann’s picture

christian.wiedemann commented
Assigned: christian.wiedemann » pdureau
pdureau’s picture

pdureau commented

I just opend the library but there are no stories inside the ui patterns in any test module.

Maybe because you wrote

$component = $this->storyPluginManager->getComponentStories($component['id']);

Instead of:

$component['stories'] = $this->storyPluginManager->getComponentStories($component['id']);
pdureau’s picture

pdureau commented
Assigned: pdureau » christian.wiedemann
Status: Needs review » Needs work
christian.wiedemann’s picture

christian.wiedemann commented

now I got it :) Mostly it is better to understand before deleting stuff:). But do you have a sample story format that I can ready test it. Inside the ui_patterns there is no sample or test component or?

christian.wiedemann’s picture

christian.wiedemann commented

Maybe we discuss this issue on thursday together with Mikael.

christian.wiedemann’s picture

christian.wiedemann commented
Status: Needs work » Needs review
pdureau’s picture

pdureau commented

Careful, I have rebased your MR. It looks mergeable to me.

But do you have a sample story format that I can ready test it.

I usually test with public implementations:

You kept the issue on your side, you want to test before merging?

Here is my list what we should change:

  • Move this hook function ui_patterns_field_config_delete(FieldConfigInterface $field_config): void TO ::UiPatternsEntitySchemaSubscriber
  • Remove ui_patterns_plugin_filter_block__ui_patterns_alter
  • Remove ui_patterns_test_theme. I think we don't need it anymore
  • Remove test components alert, close_button, prop_types_tests
  • Remove ui-patterns-actions.html.twig
  • Not sure about ui_patterns_legacy.
  • Not sure about ui_patterns_blocks.
  • Not sure about ui_patterns_field_formatters.

That's great, thanks, I have created a dedicated issue: #3493134: [2.0.0-rc1] Remove TODOs and obsolete stuff

pdureau’s picture

pdureau commented

It would be nice to add an automatic test

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Status: Needs review » Needs work

you mean adding an automatic test for stories? or which test(s)?

pdureau’s picture

pdureau commented
Assigned: christian.wiedemann » just_like_good_vibes

Those tests belongs to ui_patterns_library and must stay simple and straightforward

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Assigned: just_like_good_vibes » christian.wiedemann

i don't know why it's for me, i send it back to christian :)

pdureau’s picture

pdureau commented
Assigned: christian.wiedemann » pdureau
Status: Needs work » Reviewed & tested by the community

Auto merge

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

christian.wiedemann’s picture

christian.wiedemann commented