Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Tooling]
yarn splash
(beta) #2113[Tooling]
yarn splash
(beta) #2113Changes from all commits
08b6744
44da583
9912178
9d511ba
3adc842
a9da56e
be17151
b0c0503
90801ff
5fde385
acd9c2e
370d844
8cb4113
f7bfb82
61b233e
e50011a
204bb60
7ce1a5e
f10ffd3
7493e70
fb5019b
0db5a4b
72bc4ac
230d07c
961e3ae
4028392
af429e9
f397782
9d1873c
9b081de
3502aef
4a53045
aed6ff3
ec0fb8c
12e60e6
4a575dd
7dd8e0a
7eeba2a
768957f
1863788
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think these checks are a bit too lose. For example,
.includes('test')
will also filter file names that just happen to include those characters, even if it's not a test. I think it'd be good to be a bit more specific here for all checksThere 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.
Yeah I agree. As we make it more codebase-agnostic, this won't scale.
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.
Do you think we should filter here too?
I think, for example, if you made a change to
Avatar/index.ts
, or to autils.ts
file, I would still expect the tool to tell me accurately what the impact isThere 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.
You're right. Let me put those index and utils back in. If feedback says this is too overwhelming we can always filter them out again.
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 might not necessarily be a component
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.
I'll have to think about those edge cases, yeah.
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.
Please define these components (and really all top-level functions) using the
function Component()
style syntax. Using Arrow functions means that the components will all have no name to use for debugging which will be a pain when you get stack trace staying there was an error inUnknown
component which was insideUnknown
component.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.
I think it makes more sense to make this a
loading
boolean propThere 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.
Right now it's loading/loaded, but there can be other loading states, such as 'error' or 'cancelled' that I'd like to leave this open for.
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.
In that case can we have the vale be an enum rather than an arbitary string. There's an example of that in #2067
Then in your useState:
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.
I'm not too happy with the API here, what do you think? Any suggestions for how to improve it?
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.
I also think we can improve it. Since we'll gain insights as you explore how to make this work for GitHub, webpack, and storybook… I'd like to wait to revamp it when you know more about all these other use cases.
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.
You never set this prop to
loading
. Wouldn't you want to do that when a re-render is triggered, and you're re-running the script and the git status function?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.
Not sure what you mean by that, but let's talk about it during our next pairing session