-
Notifications
You must be signed in to change notification settings - Fork 67
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
[BD-46] feat: add testing guidelines #1165
base: master
Are you sure you want to change the base?
[BD-46] feat: add testing guidelines #1165
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Thanks for the pull request, @monteri! I've created BLENDED-1160 to keep track of it in Jira. More details are on the BD-46 project page. When this pull request is ready, tag your edX technical lead. |
Codecov Report
@@ Coverage Diff @@
## master #1165 +/- ##
==========================================
- Coverage 91.02% 90.90% -0.13%
==========================================
Files 196 195 -1
Lines 3243 3155 -88
Branches 732 704 -28
==========================================
- Hits 2952 2868 -84
+ Misses 278 274 -4
Partials 13 13
Continue to review full report at Codecov.
|
src/CheckBox/README.md
Outdated
<guide | ||
defaultText="`nextLabel: 'Next'`, `prevLabel: 'Previous'`" | ||
events="`onSelect`, `onSlid`, `onSlide`" | ||
selectors="`carousel`" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this might be copy/pasta from Carousel
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you, forgot to remove
src/CheckBoxGroup/README.md
Outdated
<guide | ||
defaultText="`nextLabel: 'Next'`, `prevLabel: 'Previous'`" | ||
events="`onSelect`, `onSlid`, `onSlide`" | ||
selectors="`carousel`" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this might be copy/paste from Carousel
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you, forgot to remove
src/Image/README.md
Outdated
@@ -46,4 +46,4 @@ notes: | | |||
<Image src="https://source.unsplash.com/1600x800/?nature,flower" fluid /> | |||
``` | |||
|
|||
|
|||
<guide /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dataTestId
is supported here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thank you, added
@@ -30,3 +30,7 @@ notes: | | |||
</Figure.Caption> | |||
</Figure> | |||
``` | |||
|
|||
<guide | |||
selectors="`figure`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dataTestId
is supported here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is supported, my mistake. Added
src/CheckBox/README.md
Outdated
@@ -91,3 +91,9 @@ class CheckBoxWrapper extends React.Component { | |||
} | |||
} | |||
``` | |||
|
|||
<guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dataTestId
is supported here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but as it is deprecated, I remove testguide
from there
www/src/scss/base.scss
Outdated
@@ -14,3 +14,4 @@ body { | |||
@import "../components/doc-elements"; | |||
@import "../components/Settings"; | |||
@import "../components/Toc"; | |||
@import "../components/TestingGuideline"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: newline
@@ -35,17 +42,28 @@ export default function PageTemplate({ | |||
h6: (props) => <LinkedHeading h="6" {...props} />, | |||
pre: props => <div {...props} />, | |||
code: CodeBlock, | |||
guide: (props) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be kind of a pain to change everywhere, but I'm thinking testguide
might be a bit clearer to make it more explicit it has to do with the testing guidelines? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is more appropriate so I change
selectors, | ||
}) => { | ||
const [collapseIsOpen, setCollapseIsOpen] = useState(false); | ||
setIsGuidelinesOnPage(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clarification] Is it OK this happens on every component re-render? Or should it be in a useEffect
with an empty dependency array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea of having useEffect
is better than simple function call. But as I read about setting state to the same value does nothing. Correct me if it is not right. But according to every render function call, yes, useEffect
is better here
@@ -0,0 +1,5 @@ | |||
.pgn-doc__guideline { | |||
code { | |||
background: #f8f8ff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an SCSS color variable we could use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I added $danger-100
. It is similar to that color
@@ -1,4 +1,4 @@ | |||
import React from 'react'; | |||
import React, {useState} from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: { useState }
a7bc1ff
to
d345511
Compare
9cc5f74
to
1aba536
Compare
1aba536
to
28551e5
Compare
New features:
<guide />
tag in the.md
files.md
file when passing props to the<guide />
tag usingmarkdown-to-jsx
JIRA: PAR-729