accordproject / web-components

React Components for Accord Project
Apache License 2.0
117 stars 94 forks source link

test(storybook): test added for headings #311

Closed sanketshevkar closed 3 years ago

sanketshevkar commented 3 years ago

Signed-off-by: User shevkar.sanket@gmail.com

Changes

Related Issues

Author Checklist

irmerk commented 3 years ago

This looks like a great start, I'll try to review it as soon as I can.

Cronus1007 commented 3 years ago

@sanketshevkar How you have selected the text since cypress doesn't have any sort of api to select the text.The first need is to write the complete selection command. @irmerk @mttrbrts I have written a little bit of code to select the text will need a little bit help in it i guess.

sanketshevkar commented 3 years ago

@sanketshevkar How you have selected the text since cypress doesn't have any sort of api to select the text.The first need is to write the complete selection command. @irmerk @mttrbrts I have written a little bit of code to select the text will need a little bit help in it i guess.

Nope. I had seen some scripts online, which could do that. I'm focusing on other tests rn as they seem to be easy and can be completed asap. If you're able get any solution do tell.

Cronus1007 commented 3 years ago

@sanketshevkar Plzzz have a look upon it. I am getting that all the tests are failing.

cd packages/storybook
npm run test:e2e
sanketshevkar commented 3 years ago

@sanketshevkar Plzzz have a look upon it. I am getting that all the tests are failing.


cd packages/storybook

npm run test:e2e

I'll look into it

irmerk commented 3 years ago

I think this looks amazing! Looks like some of these tests may be currently failing? Can you resolve and then I'll add more reviewers on this?

sanketshevkar commented 3 years ago

I think this looks amazing! Looks like some of these tests may be currently failing? Can you resolve and then I'll add more reviewers on this?

I guess it would have timed out. Because these tests were passing on local repository.

sanketshevkar commented 3 years ago

@sanketshevkarm Assertion Error has occured since the it has not found out the element.Check the Details of the failed tests.

It occurs when the storybook fails to load on the browser and thus element is not found. It works well locally.

sanketshevkar commented 3 years ago

@irmerk this issue here is that the size of headings is being different in different browsers. My local browser for the test is expecting 25px, 20px, 16px for heading 1, 2, 3 respectively. Whereas the ci/cd server expects 25px, 25px, 20px for headings 1, 2, 3 respectively. So if the test are passing on ci-cd server, it is failing on my local machine and vice-versa. I guess we'll have to normalise font-size css for headings. Would this be the right thing to do?

irmerk commented 3 years ago

I think we should be testing what is in ui-markdown-editor/src/utilities/constants, which looks like is what the ci/cd server is expecting?

Maybe even import those values into the test so we know we're testing what is the source of truth for the CSS.

sanketshevkar commented 3 years ago

@irmerk I'm really sorry for the late response. I tried importing css values from ui-markdown-editor/src/utilities/constants and it's working. Thank you for the suggestion. The problem was that with merging PR #302, the sizes of H1 and H2 are now same, the difference being their alignment. And this commit wasn't present in my local repository, so the tests were failing.

Also, I think styling for paragraph should be added in ui-markdown-editor/src/utilities/constants, because in ui-markdown-editor/src/utilities/constants style for paragraph is null.

irmerk commented 3 years ago

@sanketshevkar I believe @Michael-Grover will need to provide context on styling in relation to testing/consistency.

Michael-Grover commented 3 years ago

@irmerk @sanketshevkar what is the specific design question here? I checked the heading CSS here and on storybook and it looks consistent with what I specified when I chose the styling for these headings. I think the file linked above is the closest thing to a source of truth I can find. All of the font sizes, colors, alignment, capitalization, etc. is what I expected.

image

sanketshevkar commented 3 years ago

@irmerk @Michael-Grover I've imported the CSS constants and used them in the test as the source of truth. Requesting a review for the latest commit. Thank You.

Michael-Grover commented 3 years ago

@sanketshevkar

I took a look at the Netlify preview and it looks like there are only 3 header styles in the dropdown now image

And it looks like the style for H6 changed image

sanketshevkar commented 3 years ago

Continued on PR #350. Had some merge conflicts on my fork and local repo. Sorry for the inconvenience.