Set up ability to Add Code Documentation Associations to Lessons #39382
Conversation
dmcavoy
changed the title
dmcavoy
changed the title
| it('renders default props', () => { | ||
| const wrapper = mount(<ProgrammingExpressionsEditor {...defaultProps} />); | ||
| const wrapper = mount( |
There was a problem hiding this comment.
Why do you need mount here instead of shallow?
There was a problem hiding this comment.
It couldn't check for parts of the table without mount
There was a problem hiding this comment.
you shouldn't need to check for parts of the table; that kind of check should be handled by tests on the component that actually renders the table; the most these tests should do is check for that component
There was a problem hiding this comment.
Totally makes sense. Those tests were existing tests so I'm going to lean in favor of making them work for now and trying to get this in.
| @@ -155,6 +162,19 @@ class ProgrammingExpressionsEditor extends Component { | |||
| this.handleRemoveProgrammingExpressionDialogClose(); | |||
| }; | |||
|
|
|||
| handleOpenAddProgrammingExpression = event => { | |||
| // Prevents button from trigger save | |||
There was a problem hiding this comment.
if we specify that the button element is type="button" rather than type="submit", that should prevent this for us; see https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/button-has-type.md
There was a problem hiding this comment.
Awesome! I did not know this and it will be so helpful going forward
There was a problem hiding this comment.
When I tested this locally it did not keep the button from triggering the save
There was a problem hiding this comment.
weird. Fine to leave this for now in the interest of prompt deployment, but we should come back later and figure out what's up here and with some of the other instances of preventDefault I've seen in this space
There was a problem hiding this comment.
apps/src/lib/levelbuilder/lesson-editor/programmingExpressionsEditorRedux.js
Outdated
Show resolved
Hide resolved
| it('renders default props', () => { | ||
| const wrapper = mount(<ProgrammingExpressionsEditor {...defaultProps} />); | ||
| const wrapper = mount( | ||
| <Provider store={store}> |
There was a problem hiding this comment.
is this actually necessary, given that ProgrammingExpressionsEditor is actually UnconnectedProgrammingExpressionsEditor, as per line 5?
There was a problem hiding this comment.
the FindProgrammingExpressionDialog which is a sub component also needs the store and so it won't work without it
There was a problem hiding this comment.
is that still relevant if we switch to shallow rendering?
There was a problem hiding this comment.
If we switch to shallow rendering some of the other checks in the exists tests don't work such as looking for parts of the table
There was a problem hiding this comment.
right; hence my other comment about that. The point is that shallow rendering makes things simpler in every sense; it keeps tests focused on what they should be testing, which means both that we don't need to look inside other components to see what they're rendering and that we don't need to look inside other components to see what their rendering requirements are
There was a problem hiding this comment.
That makes sense. Since these were existing tests and this is something we would really like to have for Tuesday I'm going to keep it as is for right now. If you think we should track it to come back to we can do that.
There was a problem hiding this comment.
apps/test/unit/lib/levelbuilder/lesson-editor/programmingExpressionsEditorReduxTest.js
Show resolved
Hide resolved
| onClick={this.handleOpenAddProgrammingExpression} | ||
| color={Button.ButtonColor.orange} | ||
| /> | ||
| <FindProgrammingExpressionDialog |
There was a problem hiding this comment.
This could be something to fix in a future PR but when I try to add multiple programming expressions to a lesson it seems to save some of the state. Trying to get a good screen capture
There was a problem hiding this comment.
This is the best I can get. The search seems to be saved but the search term is cleared
Screen.Recording.2021-03-26.at.11.28.35.AM.mov
There was a problem hiding this comment.
Oh I don't think I thought about dealing with you clicking multiple. Will look.
There was a problem hiding this comment.
this is a property of the dialog itself, not of any of the code in this PR; it just requires binding state a bit closer to the value of the input rather than just listening for change events. I figured it wasn't a big enough deal, though, since the actual selection of the blocks still works
| }); | ||
|
|
||
| store = getStore(); | ||
| store.dispatch(init([], {}, [])); |
There was a problem hiding this comment.
this may be moot if we are moving off of mount / redux for this test, this line looks safe to remove. not blocking for this PR due to deadline, but worth considering next time you are in here if you are not ripping out redux entirely.
There was a problem hiding this comment.
I will make a follow up PR to update all the tests.
Hooks up the FindProgrammingExpressionDialog to the ProgrammingExpressionEditor in order to allow editors to add new programming expression associations to a lesson.
Screen.Recording.2021-03-25.at.11.03.09.PM.mov
Links
Testing story
-Manually test (see video)
PR Checklist: