Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for CoolPreview formatter. #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AaronGilMartinez
Copy link
Collaborator

No description provided.

@@ -1,4 +1,4 @@
<div>
<div class="cool-preview__wrapper">
Copy link
Collaborator Author

@AaronGilMartinez AaronGilMartinez Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to target the preview wrapper, added class.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I am not sure about these class names.
We have cool-preview__wrapper, cool-editor__dialog, cool-frame__preview.
But atm I can't think of something better, so let's review this later.

The main risk here is that people start using the class in their CSS, and then it becomes harder to change.
So we should review all the html before a stable release.

So, ok for now.

Comment on lines 87 to 88
$media->getName(),
$media->getName(),
Copy link
Collaborator Author

@AaronGilMartinez AaronGilMartinez Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template is displaying first element duplicated on a multivalued field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we should even test for cardinality > 1, as the whole system doesn't work but for the first element.

Copy link
Collaborator Author

@AaronGilMartinez AaronGilMartinez Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, removed multivalued field configuration. Add isApplicable() and readme instruccions in another branch.


$crawler = new Crawler((string) \Drupal::service('renderer')->renderRoot($build));
$elements = $crawler->filter('div.cool-preview__wrapper');
$this->assertCount(count($expected_medias), $elements);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertSameSize

$this->assertCount(1, $dialog);
$this->assertCount(1, $dialog->filter('iframe.cool-frame__preview'));
// Library for each file.
$this->assertEquals(['collabora_online/cool.previewer'], $build[$i]['#attached']['library']);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm we shouldn't check this, this way... Doesn't it bubble up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. Added a check to see that the library is present or not.

Comment on lines 87 to 88
$media->getName(),
$media->getName(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we should even test for cardinality > 1, as the whole system doesn't work but for the first element.

Copy link
Collaborator

@donquixote donquixote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some intermediate comments, will continue later.

@@ -1,4 +1,4 @@
<div>
<div class="cool-preview__wrapper">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I am not sure about these class names.
We have cool-preview__wrapper, cool-editor__dialog, cool-frame__preview.
But atm I can't think of something better, so let's review this later.

The main risk here is that people start using the class in their CSS, and then it becomes harder to change.
So we should review all the html before a stable release.

So, ok for now.

Comment on lines 34 to 36
protected static $modules = [
'collabora_online',
'key',
'media',
'user',
'field',
'system',
'file',
'image',
'entity_test',
];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to repeat modules from the parent class.

Suggested change
protected static $modules = [
'collabora_online',
'key',
'media',
'user',
'field',
'system',
'file',
'image',
'entity_test',
];
protected static $modules = [
'entity_test',
];


// Create test file.
file_put_contents('public://file-1.txt', $this->randomString());
File::create(['uri' => 'public://test-1.txt'])->save();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the '-1' in the example file name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover, changing the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants