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

Display check failure message #1451

Merged
merged 2 commits into from
May 26, 2023
Merged

Conversation

nelsonkopliku
Copy link
Member

Description

This PR adds rendering of a check expectation's failure_message returned by wanda on a failing check.

Took the chance to tidy up a bit factories. More might come as we go.

@jagabomb I went for option 1 in figma just out of personal taste, however let me know if option two should be the way.

image

How was this tested?

Automated tests/storybook

@nelsonkopliku nelsonkopliku added enhancement New feature or request user story ux javascript Pull requests that update Javascript code labels May 24, 2023
@nelsonkopliku nelsonkopliku self-assigned this May 24, 2023
@nelsonkopliku nelsonkopliku force-pushed the display-check-failure-message branch from 2b32b04 to 69475bb Compare May 24, 2023 13:04
Copy link
Contributor

@abravosuse abravosuse left a comment

Choose a reason for hiding this comment

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

Code apart, that looks awesome to me.

@nelsonkopliku nelsonkopliku force-pushed the display-check-failure-message branch from 69475bb to 33b4197 Compare May 24, 2023 13:33
@jagabomb
Copy link
Contributor

jagabomb commented May 24, 2023

@jagabomb I went for option 1 in figma just out of personal taste, however let me know if option two should be the way.

Thanks for this @nelsonkopliku, Option 1 is a good option to proceed with. The only question I have is, is it possible to emphasis certain values in the failing message, like the words Failing and values (3 and 1)?

@nelsonkopliku
Copy link
Member Author

is it possible to emphasis certain values in the failing message, like the words Failing and values (3 and 1)?

@jagabomb I was waiting for this 😄
So, we could make Failing bold/semibold without much hassle, however consider that in the same result there might also be Passing evaluations. What about those?

In regards to highlighting specific parts of the failure message I am afraid that would be trickier.
It would mean making failure_message field exposed by wanda a markdown possibly, which would bring in the problem of interpolation inside a markdown and refactoring all the checks.

Honestly, not an option for me at the moment 😅

@jagabomb
Copy link
Contributor

After thinking of the Passing state, it would not make sense to emphasise the Failing/Passing wording. So let's just scrap my request there.

I was hoping we could do some interpolation on the frontend to take the whatever is within quotations ('') and make those values bold. But if it too much heavy effort for now then lets proceed with the initial PR. 😅

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@nelsonkopliku Good!
I just proposed some changes in the factory itself.
We don't need to do such a default values handling, fishery is pretty smart with those things.
Don't need a new review anyway

assets/js/lib/test-utils/factories/executions.js Outdated Show resolved Hide resolved
assets/js/lib/test-utils/factories/executions.js Outdated Show resolved Hide resolved
assets/js/lib/test-utils/factories/executions.js Outdated Show resolved Hide resolved
@nelsonkopliku nelsonkopliku force-pushed the display-check-failure-message branch from 33b4197 to 82c2b73 Compare May 25, 2023 07:17
@nelsonkopliku nelsonkopliku force-pushed the display-check-failure-message branch from 82c2b73 to 0bbcf62 Compare May 25, 2023 07:31
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

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

Approved as per comments.

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey man nice!
LGTM

@nelsonkopliku nelsonkopliku force-pushed the display-check-failure-message branch from 0bbcf62 to ff1f4a2 Compare May 25, 2023 13:28
@nelsonkopliku nelsonkopliku merged commit da4651b into main May 26, 2023
@nelsonkopliku nelsonkopliku deleted the display-check-failure-message branch May 26, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code user story ux
Development

Successfully merging this pull request may close these issues.

5 participants