-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Refactor frontend main page and badge-example code #2441
Conversation
Generated by 🚫 dangerJS |
b9e63fd
to
debfa46
Compare
This pull request introduces 1 alert when merging debfa46 into 0a6afb4 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging ea2f1a4 into 0a6afb4 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 2 alerts when merging b475529 into 0a6afb4 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
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.
Nice one. A few minor bits. I think the main problematic thing is the documentation not rendering in the modals.
} | ||
|
||
asNative() { | ||
return this.definitionData |
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 find this method name a bit confusing
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.
Would toArray()
be better?
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.
yeah, that seems more descriptive
Co-Authored-By: paulmelnikow <[email protected]>
This pull request introduces 2 alerts when merging 4409563 into 8a8311d - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 2 alerts when merging 5240559 into 8a8311d - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging 74741f7 into 8a8311d - view on LGTM.com new alerts:
Comment posted by LGTM.com |
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.
think this is good to go
Awesome. Thanks for reviewing! |
Amazing work with all of this @paulmelnikow! 👏 |
:user
or:gem
, a minor tweak discussed at namedParams are not interpolated in examples #2427 (comment).It's a medium-sized refactor:
BadgeExamples
,Category
andBadge
component with a completely rewrittenBadgeExamples
component which renders a table of badges, andCategoryHeading
andCategoryHeadings
components.ExamplesPage
andSearchResults
components into a newMain
component.MarkupModal
. Rather than rely on unmounting and remounting the component to copy the badge URL into state, employ thegetDerivedStateFromProps
lifecycle method.prepareExamples
andall-badge-examples
.$suggest
schema to harmonize with the service definition format. It's not backward-compatible which means at deploy time there probably will be 10–20 minutes of downtime on that feature, between the first server deploy and the final gh-pages deploy. 🤷♂️ (We could leave the old version in place if it seems worth it.)make-badge-url
with tests. I removed most of the uses of the old functions, but there are some in parts of the frontend I didn't touch like the static and dynamic badge generators, and again I didn't want to make this any larger than it already is.