Tackling late-arriving comments from #5186 #5626
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mistercrunch
force-pushed
the
chris_comments
branch
from
1829a0c to
2d10bf0
Compare
Codecov Report
@@ Coverage Diff @@
## master #5626 +/- ##
==========================================
- Coverage 63.48% 63.48% -0.01%
==========================================
Files 360 360
Lines 22889 22891 +2
Branches 2547 2547
==========================================
+ Hits 14531 14532 +1
- Misses 8343 8344 +1
Partials 15 15
Continue to review full report at Codecov.
|
williaster
approved these changes
Aug 14, 2018
There was a problem hiding this comment.
Thanks @mistercrunch for taking the time to come back to this! LGTM.
+1 to turning on the linting rule, frontend folks here push it and I think it results in being more explicit/less error prone.
|
Also I think this chevron pattern (point in the direction things will go) is well-established, google uses it which seems like a strong indicator.
|
|
Ok, I'll try and find the time to change the caret / chevron! |
wenchma
pushed a commit
to wenchma/incubator-superset
that referenced
this pull request
Nov 16, 2018
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
@williaster thanks for the review, tackled most of your comments from #5186 here
Also created issue for related to isRequired/default props here: #5625 . I had the habit to not set a default when default would be
null, typically forPropTypes.node. Happy being more explicit about it.