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

refactor: [M3-6263] - MUI v5 Migration - SRC > Features > EntityTransfers #9582

Merged
merged 14 commits into from
Sep 6, 2023
Merged

refactor: [M3-6263] - MUI v5 Migration - SRC > Features > EntityTransfers #9582

merged 14 commits into from
Sep 6, 2023

Conversation

tyler-akamai
Copy link
Contributor

@tyler-akamai tyler-akamai commented Aug 22, 2023

Description 📝

MUI v5 CSS migration for SRC > Features > EntityTransfers

Major Changes 🔄

  • Removed React.FC or `
  • Renamed interface Props to {ComponentName}Props.
  • Removed CombinedProps if it's directly referencing Props with nothing additional
  • Removed makeStyles for new styled API.
  • export component inline and removed export default {ComponentName} at bottom of file.

Preview 📷

Before After
Screenshot 2023-08-23 at 4 46 02 PM Screenshot 2023-08-23 at 4 46 10 PM
Screenshot 2023-08-23 at 4 47 10 PM Screenshot 2023-08-23 at 4 47 22 PM
Screenshot 2023-08-23 at 4 47 42 PM Screenshot 2023-08-23 at 4 48 07 PM

How to test 🧪

  1. Navigate to http://localhost:3000/account/service-transfers
  2. Ensure the functionality and styling is consistent

@tyler-akamai tyler-akamai changed the title feat: [M3-6263] - MUI v5 Migration - SRC > Features > EntityTransfers refactor: [M3-6263] - MUI v5 Migration - SRC > Features > EntityTransfers Aug 22, 2023
@tyler-akamai tyler-akamai self-assigned this Aug 22, 2023
abailly-akamai
abailly-akamai previously approved these changes Aug 23, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! Approving pending some cleanup

@abailly-akamai abailly-akamai requested review from abailly-akamai and removed request for abailly-akamai August 23, 2023 11:58
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

oops i missed this was a draft - comments still apply tho :)

@abailly-akamai abailly-akamai dismissed their stale review August 23, 2023 12:00

This is a draft

@tyler-akamai tyler-akamai marked this pull request as ready for review August 23, 2023 20:54
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Awesome job! Just a few things:

When there are no linodes in the /account/service-transfers/create page, the table column widths are different on your branch vs prod:

branch:
image

prod:
image

Also, it's not really easy to screenshot so I've only included an image of the area, not a comparison, but the Service Transfer Summary area on the same page may be slightly shifted? Will say though that sometimes I've come across spacing issues like this that just... disappeared the next day (???) so it may not be anything, but worth double checking:
image

label: 'StyledDebouncedSearchTextField',
})(({ theme }) => ({
marginBottom: theme.spacing(0.5),
maxWidth: 556,
Copy link
Contributor

Choose a reason for hiding this comment

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

😆 I wonder how they got this specific of a number

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Still getting a slight difference in width if there are no linodes for the /service-transfers/create page on this branch vs prod, seems to be the checkbox column. Other than that everything else looked good!

image

prod
image

@tyler-akamai
Copy link
Contributor Author

Still getting a slight difference in width if there are no linodes for the /service-transfers/create page on this branch vs prod, seems to be the checkbox column. Other than that everything else looked good!

image

prod image

Yeah its a little wider without data, but as soon as data is added, it expands to the width I currently have. Its a minor difference but I wouldn't consider it a regression.

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Ok thanks for the clarification, approving in that case 🎉

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Aug 31, 2023
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Looks like search functionality in Make a Service Transfer page is not working as I expected.

Local Production
image image

Comment on lines +88 to +105
acceptEntityTransfer(token)
.then(() => {
// @analytics
if (data?.entities) {
const entityCount = countByEntity(data?.entities);
sendEntityTransferReceiveEvent(entityCount);
}
// Update the received transfer table since we're already on the landing page
queryClient.invalidateQueries({
predicate: (query) =>
query.queryKey[0] === queryKey &&
query.queryKey[2] === TRANSFER_FILTERS.received,
});
onClose();
setSubmitting(false);
enqueueSnackbar('Transfer accepted successfully.', {
variant: 'success',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have: we could simplify this with async/await's

@coliu-akamai
Copy link
Contributor

@cpathipa if you're using the MSW, I'm not too sure if the search bar filters the MSW linodes: I'm getting the same behavior on the develop branch, where I can't search/filter MSW linodes. However, I tried turning off the MSW and creating some linodes, and the searchbar seemed to work fine for the actual linodes -- could you confirm this behavior on your end?

@cpathipa
Copy link
Contributor

cpathipa commented Sep 6, 2023

@cpathipa if you're using the MSW, I'm not too sure if the search bar filters the MSW linodes: I'm getting the same behavior on the develop branch, where I can't search/filter MSW linodes. However, I tried turning off the MSW and creating some linodes, and the searchbar seemed to work fine for the actual linodes -- could you confirm this behavior on your end?

@coliu-akamai My bad, I didn't realize MSW was enabled locally. Confirming that the search bar is working as I expected. and approved the PR with pending minor feedback on using async/await's.

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 6, 2023
@tyler-akamai tyler-akamai merged commit ed32421 into linode:develop Sep 6, 2023
11 checks passed
abailly-akamai pushed a commit that referenced this pull request Sep 7, 2023
…fers (#9582)

* initial migration commit

* finished EntityTransfers/ and EntityTransfersCreate/

* Partly done EntityTransfersLanding directory

* finished css migration, have not tested yet

* Added changeset: MUI v5 Migration - SRC > Features > EntityTransfers

* removed test code

* testing formating issues

* fixed inconsistent styling issues

* fixed pr suggestions

---------

Co-authored-by: TylerWJ <tylerwjones99@gmail.com>
@tyler-akamai tyler-akamai deleted the M3-6263-Entity-Transfer-CSS-Migration branch September 8, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants