♻️ amp-story: Refactor JSX#36706
Merged
alanorozco merged 46 commits intoampproject:mainfrom Nov 30, 2021
Merged
Conversation
|
Hey @gmajoulet, @newmuis! These files were changed: |
713bf92 to
c87e8c6
Compare
This comment has been minimized.
This comment has been minimized.
mszylkowski
reviewed
Nov 2, 2021
jridgewell
reviewed
Nov 2, 2021
gmajoulet
approved these changes
Nov 3, 2021
Contributor
gmajoulet
left a comment
There was a problem hiding this comment.
0.4 more kb shaved!!
I didn't read all the code that closely but the overall direction is awesome, thank you Alan.
fcfac23 to
393b0d8
Compare
This comment has been minimized.
This comment has been minimized.
|
This pull request introduces 1 alert when merging 398e75e into 2d5d9a2 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 1790841 into 2d5d9a2 - view on LGTM.com new alerts:
|
1790841 to
e7f2b2f
Compare
|
This pull request introduces 1 alert when merging e7f2b2f into d287103 - view on LGTM.com new alerts:
|
Member
|
Does this JSX work in classic AMP? |
Member
Author
|
@samouri Yep! |
|
This pull request introduces 1 alert when merging d3c08b4 into 27754c5 - view on LGTM.com new alerts:
|
d3c08b4 to
721ef27
Compare
|
This pull request introduces 1 alert when merging 721ef27 into ce5decd - view on LGTM.com new alerts:
|
721ef27 to
a649760
Compare
processprocess
approved these changes
Nov 5, 2021
| // We set the href imperatively here to avoid triggering the warning | ||
| // (an lgtm-disable comment will not work due to formatting, unfortunately.) | ||
| // Consider whether we can use `noopener` and/or `noreferrer` so that we | ||
| // can inline the attribute, and avoid the warning. |
Contributor
There was a problem hiding this comment.
Thank you for leaving a comment explaining this!
3591485 to
82f702b
Compare
a101f4a to
d758312
Compare
This comment has been minimized.
This comment has been minimized.
cf1dc76 to
4d2f064
Compare
42692d2 to
eebe544
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Refactor to take advantage of JSX inline patterns. Rendering functions were simplified were possible. Local instance members were removed in cases where inlining was simpler.
This simplifies the model of rendering functions, while shrinking the
amp-storybundle by0.90 kbin compressed size.