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

Delete bridges from the UI #4046

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Conversation

jkongie
Copy link
Contributor

@jkongie jkongie commented Mar 9, 2021

  • Replaces loading data using redux with hooks in Bridges#Show
  • Adds table row clicking to the Bridges List
  • Display the Bridge data in a table format for improved readability
  • Add a button to delete a bridge on the Bridges#Show page

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2021

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@jkongie jkongie force-pushed the feature-176137722/delete-bridge-from-ui branch from 1f37025 to a9e0956 Compare March 9, 2021 19:17
@jkongie jkongie requested a review from DeividasK March 10, 2021 04:25
@jkongie jkongie force-pushed the feature-176137722/delete-bridge-from-ui branch from 7c0a5e2 to 19e7d2f Compare March 10, 2021 09:30
DeividasK
DeividasK previously approved these changes Mar 15, 2021
Copy link
Collaborator

@DeividasK DeividasK left a comment

Choose a reason for hiding this comment

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

2 questions, but LGTM 👍

Comment on lines +3 to +9
// Contains styles to make a table row linkable
//
// This has been placed in the components folder because we ideally want to
// wrap the link in a reusable components, however using Material UI 3
// 'withStyles' leads to small freeze when navigating to the link, due to
// the slowness of JSS. When we migrate to MUI 4/5 we can implement this with
// makeStyles which doesn't have those problems
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also use a regular style objects instead of createStyles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was attempting to attempting to customise the TableRow itself using this HOC technique

https://v3.material-ui.com/customization/overrides/#shorthand

However the problem can be found I found was that it spends too much time in JSS.

mui/material-ui#10778 (comment)

I could use styles to do it, but I'm hoping we can upgrade this to MUI v4 soon, and then make the improvement.

Comment on lines 9 to 15
const mockHistoryPush = jest.fn()
jest.mock('react-router-dom', () => ({
...(jest.requireActual('react-router-dom') as typeof ReactRouterDom),
useHistory: () => ({
push: mockHistoryPush,
}),
}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need to mock it? Because we are using a memory router in mountWithProviders it should be perfectly fine to simply inspect the location without having to mock react-router-dom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thanks for the tip

I've removed the mock and added the location check

* Replaces loading data using redux with hooks in Bridges#Show
* Adds table row clicking to the Bridges List
* Display the Bridge data in a table format for improved readability

https://www.pivotaltracker.com/story/show/176137722
@jkongie jkongie requested a review from DeividasK March 15, 2021 10:57
@jkongie jkongie merged commit b8d87ac into develop Mar 15, 2021
@jkongie jkongie deleted the feature-176137722/delete-bridge-from-ui branch March 15, 2021 11:05
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.

2 participants