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.

Sign up for GitHub

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

Jump to bottom

Address XS code hygiene tasks #4981

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Address XS code hygiene tasks #4981

wants to merge 12 commits into from

Conversation

OEvgeny
Copy link
Collaborator

@OEvgeny OEvgeny commented Dec 21, 2023 •
edited
Loading

part of #4968

Changelog Entry

(No changelog entry for code hygiene tasks)

Description

Address the first 3 suggestions from #4968

Design

Specific Changes

List of changes:

  • XS: Do not use React FC/VFC types, use Props instead
  • XS: Mark all props as read-only, also any arrays and objects under it
  • XS: Prefer import { type ABC } over import type { ABC }

-

  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@OEvgeny
Copy link
Collaborator Author

OEvgeny commented Jan 2, 2024

@compulim if anything is missing, please let me know!

OEvgeny
OEvgeny commented Jan 9, 2024
View reviewed changes
__tests__/types/tsconfig.json Outdated Show resolved Hide resolved
@OEvgeny
Address XS code hygiene tasks
c19c045 
Address the first 3 suggestions from #4968

As we remove `FC/VFC` I thought it'd be nice to add `ReactNode` return type basically to:
- Serve as an additional hint that the function follows React Component signature
- Limit usage of unsupported by React runtime return types
@OEvgeny
import type -> import { type } and VFC removal
691d3ec
@OEvgeny
More Readonly to the god of Readonly
05ae57d
@OEvgeny
Fix PropsWithChildren<{}> allows any props
1fc36a3
@OEvgeny
Get rid of nested Readonly to make tests pass
dd9de64
@OEvgeny
Remove redundant Readonly
54774db
@OEvgeny
Add lint rule for inner { type } imports and fix all
830ceee
OEvgeny
OEvgeny commented May 1, 2024
View reviewed changes
Copy link
Collaborator Author

@OEvgeny OEvgeny left a comment

Choose a reason for hiding this comment

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

Went through the files and fixed the following:

  • Removed PropsWithChildren
  • Sorted imports and props

@OEvgeny OEvgeny mentioned this pull request May 17, 2024
Open
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n a-b-r-o-w-n is a code owner

@compulim compulim Awaiting requested review from compulim compulim is a code owner

@cwhitten cwhitten Awaiting requested review from cwhitten cwhitten is a code owner

@srinaath srinaath Awaiting requested review from srinaath srinaath is a code owner

@tdurnford tdurnford Awaiting requested review from tdurnford tdurnford is a code owner

@beyackle2 beyackle2 Awaiting requested review from beyackle2 beyackle2 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@OEvgeny