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

Adds transcript form #25

Merged
merged 13 commits into from
Sep 12, 2019
Merged

Adds transcript form #25

merged 13 commits into from
Sep 12, 2019

Conversation

allishultes
Copy link
Contributor

Is your Pull Request request related to another issue in this repository ?

Issue #7

Describe what the PR does

State whether the PR is ready for review or whether it needs extra work

Ready

Additional context

Outstanding question: I'm not sure the CustomAlert should be a part of the TranscriptForm; when in the cycle would the TranscriptForm know that a transcription has failed? Because the ApiWrapper has now been decoupled, and that logic left to a parent component, we probably wouldn't pass error messages back down...? @jamesdools and @emettely let me know what you think.

@allishultes allishultes added the generic component A non-workspace component for the storybook label Sep 10, 2019
packages/components/FormModal/index.js Outdated Show resolved Hide resolved
};

const setAlert = () => {
if (showAlert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic of showAlert should be in the place where it's importing this CustomAlert. I wonder if in storybooks you can add a button to toggle the alert to come up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh yeah - I think that's the knobs. I'll try and add one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm not sure knobs work as expected with functional components — I've managed to allow users to toggle a button to turn the alert on and off, but the component doesn't re-render when the props change using the add-on.

I have separated the CustomAlert from the TranscriptCard; I think you're right in that that will be handled by the parent component.

Copy link
Contributor

Choose a reason for hiding this comment

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

The showAlert should probably be a state of the parent component 🤔 which also implies that maybe the actual handling functions should also be in the parent component.

packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
packages/components/TranscriptForm/index.js Outdated Show resolved Hide resolved
@allishultes
Copy link
Contributor Author

Some comments re: the code review:

  • I'm unsure whether Storybook Knobs, which allow users to control props, etc ‚ works with functional components. I spent a few hours trying to set up a knob that lets them turn CustomAlert on/off, but didn't have much success. I'll make a card about looking further into it for the backlog.
  • I've pulled CustomAlert out of the TranscriptForm — I think you're right, this should be handled by the wrapper.
  • I also didn't see file.path ever being used so I've removed it.
  • I merged withmaster and also cleaned up a few console errors after you finished your review, @emettely so that's what's up with new files :)

Copy link
Contributor

@emettely emettely left a comment

Choose a reason for hiding this comment

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

suggestion: ShowAlert in parent component as state? Maybe this is a bigger refactor step. Might be something we can put off for a bit. Everything else seems good though! Just wanted to get your thoughts on it :)

};

const setAlert = () => {
if (showAlert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The showAlert should probably be a state of the parent component 🤔 which also implies that maybe the actual handling functions should also be in the parent component.

@allishultes
Copy link
Contributor Author

suggestion: ShowAlert in parent component as state? Maybe this is a bigger refactor step. Might be something we can put off for a bit. Everything else seems good though! Just wanted to get your thoughts on it :)

No definitely agree! I think a step further down the line because so far, there isn't a parent component in Storybook. But agree 100%

@allishultes allishultes merged commit df3069c into master Sep 12, 2019
@allishultes allishultes deleted the adds-transcript-form branch September 12, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generic component A non-workspace component for the storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants