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

removed tss-react from cord-field - 3466480946 #1164

Open
wants to merge 238 commits into
base: develop
Choose a base branch
from

Conversation

andrewmurraydavid
Copy link
Member

@andrewmurraydavid andrewmurraydavid commented Sep 14, 2022

@andrewmurraydavid andrewmurraydavid force-pushed the remove-tss-react branch 2 times, most recently from dd89666 to 1f396fb Compare September 14, 2022 20:39
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

I think I like this. It does "clutter" the jsx a bit more, but I think it's better to have those styles close to their usage. I've often found myself asking "what does this class do" and having to jump back and forth between styling at top and usage in jsx tree.

So I'm good with this if you like it 👍

@CarsonF
Copy link
Member

CarsonF commented Sep 14, 2022

Will you squash/fixup that second commit as well?

@andrewmurraydavid
Copy link
Member Author

@CarsonF
What second commit? If you're talking about the one with the yarn updates, that has been removed.These 2 commits here are only with the tss-react changes

@CarsonF
Copy link
Member

CarsonF commented Sep 14, 2022

These 2 commits here are only with the tss-react changes

Yeah I'd just prefer that "clean up" commits get squashed so we are left with single purpose commits.

src/components/Error/Error.tsx Outdated Show resolved Hide resolved
src/components/List/List.tsx Outdated Show resolved Hide resolved
src/components/List/List.tsx Outdated Show resolved Hide resolved
src/components/List/List.tsx Outdated Show resolved Hide resolved
src/scenes/Users/List/UserList.tsx Outdated Show resolved Hide resolved
src/scenes/Users/List/UserList.tsx Outdated Show resolved Hide resolved
@andrewmurraydavid andrewmurraydavid changed the title removed tss-react from cord-field WIP removed tss-react from cord-field Sep 15, 2022
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

Here's some comments to get started on. The idea behind each comment could be applied across the board so take a look at the meaning and see where else it could be applied.

src/components/BooleanProperty/BooleanProperty.tsx Outdated Show resolved Hide resolved
src/components/Breadcrumb/Breadcrumb.tsx Outdated Show resolved Hide resolved
src/components/Changeset/ChangesetIcon.tsx Outdated Show resolved Hide resolved
@@ -1,59 +1,19 @@
import { Badge, Grid, TooltipProps, Typography } from '@mui/material';
import { Badge, Grid, Theme, TooltipProps, Typography } from '@mui/material';
Copy link
Member

Choose a reason for hiding this comment

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

The styling changes in this file are overly complicated and doing incorrect things. I'll expand/help later.

src/components/Changeset/ChangesetBanner.tsx Outdated Show resolved Hide resolved
src/components/Changeset/PropertyDiff.tsx Outdated Show resolved Hide resolved
@andrewmurraydavid andrewmurraydavid marked this pull request as ready for review October 25, 2022 18:58
@andrewmurraydavid andrewmurraydavid changed the title WIP removed tss-react from cord-field removed tss-react from cord-field Oct 25, 2022
@sethmcknight sethmcknight linked an issue Nov 2, 2022 that may be closed by this pull request
@sethmcknight sethmcknight changed the title removed tss-react from cord-field 3466480946 removed tss-react from cord-field Nov 2, 2022
@sethmcknight sethmcknight changed the title 3466480946 removed tss-react from cord-field removed tss-react from cord-field - 3466480946 Nov 2, 2022
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.

Remove tss-react from cord-field
3 participants