Spec page #79
Spec page #79
Conversation
PatrickShaw
requested review from
PatrickShaw,
calebj0seph,
darvid7 and
TimGebert
|
This doesn't work for me... Could we also get a description of what is being added plz |
|
Agreed, would like a description of what's changed exactly and for the actual code to work before reviewing. |
| @@ -50,6 +53,18 @@ export const Page: ComponentType<{}> = () => ( | |||
| <Route path="/settings" component={SettingsViewer} /> | |||
| <Route component={NotFound} /> | |||
| </Switch> | |||
| <Route exact path="/specs/:specId/edit" component={AddSpecModal} /> | |||
There was a problem hiding this comment.
I realise this is kind of messy, but these routes no longer work in the Overview component since they're all triggered from the /specs/... route now, and I couldn't get these Route components to work inside SpecViewer since I couldn't extract the specId match prop, since it's part of the match in Page.
I probably just don't understand Routes well enough though, let me know if there's a cleaner way to do this.
There was a problem hiding this comment.
Have you tried doing something like <Route path={`${match.path}/edit`} component={AddSpecModal}/> inside SpecViewer? This should let you access match.params.id inside AddSpecModal.
There was a problem hiding this comment.
UI looks and works well. Functionality also appears to work with backend. 👍
Can't really comment on the code for frontend, can @calebj0seph @PatrickShaw @darvid7 please get this through
'Run all' button still does nothing, but I understand that is part of a separate task?
Good work.
| @@ -50,6 +53,18 @@ export const Page: ComponentType<{}> = () => ( | |||
| <Route path="/settings" component={SettingsViewer} /> | |||
| <Route component={NotFound} /> | |||
| </Switch> | |||
| <Route exact path="/specs/:specId/edit" component={AddSpecModal} /> | |||
There was a problem hiding this comment.
Have you tried doing something like <Route path={`${match.path}/edit`} component={AddSpecModal}/> inside SpecViewer? This should let you access match.params.id inside AddSpecModal.
| @@ -40,7 +40,9 @@ export class DeleteSpecModal extends Component<RouteComponentProps<{ specId: str | |||
| }; | |||
|
|
|||
| private closeModalOnDelete = () => { | |||
| this.props.history.push(goUpUrl(this.props.match.url, 2)); | |||
| this.props.history.push( | |||
| goUpUrl(this.props.match.url.replace('specs', 'overview'), 2), | |||
There was a problem hiding this comment.
How does this work? Why use replace instead of appending overview to the end of the base URL?
What if we're closing the modal from a page other than the overview pages - Won't it go to the overview page instead of the original page?
There was a problem hiding this comment.
Because it’s delete - if we’re deleting the entire spec we can’t stay on the spec page, because there’ll be nothing there.
Juat dismissing the modal without deleting won’t go to overview.
There was a problem hiding this comment.
Why not just do this.props.history.push(goUpUrl(this.props.match.url, 3) + '/overview')?
The problem with the way you've got it at the moment is that if you have a base url that has "specs" in it, say, openapi.specsavers.com/specs/1/delete you'll end up with openapi.overviewavers/overview.
|
What's the use case for having a separate spec page? From what I can see, it seems to provide the same functionality as the overview page. |
Proposed changes
Types of changes
What types of changes does your code introduce to OpenAPI Platform?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...