-
Notifications
You must be signed in to change notification settings - Fork 4
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
[WEB-3649] Port JSX to TSX #326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing, great work, I have just a couple of suggestions, otherwise believe it can be merged
src/core/CustomerLogos/component.tsx
Outdated
import React from "react"; | ||
|
||
type CustomerLogosProps = { | ||
companies: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a some kind of company entity type here ?
public/mockServiceWorker.js
Outdated
@@ -95,7 +95,7 @@ self.addEventListener("fetch", function (event) { | |||
|
|||
// Bypass navigation requests. | |||
if (request.mode === "navigate") { | |||
return; | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter strikes again. I added to the .prettierignore
to exclude this file, but something slipped through. I shall update to remove this from the diff
<p className="ui-meganav-media-heading"> | ||
Status | ||
<iframe | ||
title="Ably status" | ||
src="https://status.ably.com/embed/icon" | ||
allowtransparency="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, this thing is being taken care of, it really annoyed me 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird how React and the DOM have no real way to agree on this. React hates this form, the DOM hates allowTransparency
Jira Ticket Link / Motivation
https://ably.atlassian.net/browse/WEB-3649
Summary of changes
Takes every JSX file in this repo and converts it to TSX, introducing additional supporting types that are present across the assets.
I only performed the minimum changes, that is, rename the file extension and do what is needed to appease TSC - no other logical changes have been made (where possible).
There's a host of JS assets as well, but this repo is big enough.
How do you manually test this?
Pull down, pull packages,
yarn storybook
, it works.Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist