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

Text | Testing | Improved documentation and tests #1137

Merged
merged 23 commits into from
Apr 12, 2019

Conversation

mikemai2awesome
Copy link
Collaborator

@mikemai2awesome mikemai2awesome commented Mar 29, 2019

Jira

http://vjira2:8080/browse/BDS-968

Summary

Added tests and docs for the private bolt-text web component.

Details

  1. text.js in the tests folder now contains tests for all props.
  2. testing.md now has detailed instructions on manual testing.
  3. Docs have been rewritten to reflect latest format on demonstrating web components.

How to test

Run the branch locally and read through the testing javascript and markdown.

NOTE

The doc pages are using a page template, which makes sense in this case, because web component demos are different, the actual markup for each of the examples are laid out on each page.

cc @sghoweri

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

I approved this PR. Examples look great, tests are passing. 👍

I found a couple bugs that should probably be fixed separately.

  1. There is a little extra whitespace around the inline text example. This is probably coming from the JS, as it was with the link component.
    image

  2. The snapshots are different between the shadow vs no-shadow versions of the 'xxxlarge' headline test. They should match, but the one w/ shadow is larger. This tells me there's some CSS that needs to be fixed.
    image

Would you make tickets for these? You can kick back to me to make the tickets if you think that makes more sense.

Copy link
Collaborator

@adamszalapski adamszalapski left a comment

Choose a reason for hiding this comment

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

I agree with @danielamorse that is a difference in xxl size snapshot with shadow vs no shadow and in the inline text is a small space in the right. But in overall is great wor @mikemai2awesome. I have on the comment regarding unused imports and variable.

@@ -0,0 +1,207 @@
import { render } from '@bolt/twig-renderer';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The good practice is removing unused imports and variables like:
line 1: import { render } from '@bolt/twig-renderer';
line 7-21: const { tag, display, color, align, opacity, quoted, 'line-height': lineHeight, 'letter-spacing': letterSpacing, 'text-transform': textTransform, 'font-family': fontFamily, 'font-size': fontSize, 'font-style': fontStyle, 'font-weight': fontWeight, } = schema.properties;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danielamorse Can you read about the comment above? Should 7-21 be removed? I am not sure because you helped me set those up. :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mikemai2awesome yes, you can remove any imports and variables you are not using. In VS Code unused variables get dimmed-out. Anything similar on your end? If not you can always just search the file for those variables. In the case of variables like 'line-height': lineHeight, you would search for lineHeight to see if it's being used.

I just added them all initially because I didn't know which ones you'd want to test. Line 1 (render import) can be removed, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mikemai2awesome I removed the unused vars and fixed the snapshot issue.

@adamszalapski the snapshot issue was an interesting JS bug. See this ticket for an explanation: http://vjira2:8080/browse/BDS-1222. Can you please review these commits and let me know if it all looks good to you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamszalapski I requested review again. See latest commits from @danielamorse

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch @danielamorse, i have one question to tests. Right now quoted test snapshot looks different than the quoted example from PL.
image
image
On both cases with and without shadow DOM. The open quotation marks are missing.

Copy link
Collaborator

@adamszalapski adamszalapski left a comment

Choose a reason for hiding this comment

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

Please see my last comment

@mikemai2awesome
Copy link
Collaborator Author

@danielamorse @adamszalapski

I have centered the text so the snapshots can show correctly.

@danielamorse danielamorse merged commit 55aa2bb into master Apr 12, 2019
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.

4 participants