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

feat(@clayui/empty-state): Output a reduced motion image by default #5640

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

pat270
Copy link
Member

@pat270 pat270 commented Jul 18, 2023

fixes #5638

@matuzalemsteles I didn't update the tests yet. I'm having trouble writing it for:

Jest: "./packages/clay-empty-state/src/" coverage threshold for statements (100%) not met: 92.85%
Jest: "./packages/clay-empty-state/src/" coverage threshold for lines (100%) not met: 92.85%
Jest: "./packages/clay-empty-state/src/" coverage threshold for functions (100%) not met: 50%

I'm not sure what statements, lines, and functions mean.

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

@pat270 I added some comments about some things we can change.

I'm not sure what statements, lines, and functions mean.

Oh yeah, basically the tests aren't covering all the edge cases that exist, for example if a test case created to test the behavior of the component didn't pass a condition, it called a function or line in that path that decreased the coverage percentage of the component test.

You can run yarn test --coverage to get the coverage of the tests, a folder is created in the root of Clay with a minimal static website that you can use to browse and track coverage and check which lines or conditions of code do not are covered by tests.

packages/clay-empty-state/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-empty-state/src/index.tsx Outdated Show resolved Hide resolved
@pat270 pat270 marked this pull request as ready for review July 20, 2023 23:14
@pat270
Copy link
Member Author

pat270 commented Jul 20, 2023

@matuzalemsteles I've been trying to test onError and the warn message, but not sure how to go about it to meet the coverage threshold.

@matuzalemsteles
Copy link
Member

@pat270 no problem, we can decrease the coverage limit, you can edit the value by changing it in the jest.config.js file. https://github.com/liferay/clay/blob/master/jest.config.js#L88-L92

: `https://${location.host}`
);

const index = url.pathname.match(/.(gif|png| jpeg|jpg)/)?.index;
Copy link
Member

Choose a reason for hiding this comment

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

@pat270 just to avoid use cases where the URL has a link to the image but doesn't have the file extension, we can return null instead of still trying to infer the URL.

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

LGTM

@matuzalemsteles matuzalemsteles merged commit 7ef579f into liferay:master Jul 26, 2023
2 checks passed
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.

@clayui/empty-state: Append _reduced_motion to the imgSrc url by default
2 participants