-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Use Preact for asynchronous rendering for /settings/integrations #986
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
Use Preact for asynchronous rendering for /settings/integrations #986
Conversation
padding: 3px 0px; | ||
margin-top:-5px; | ||
float: right; | ||
&:hover, &:focus { |
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 prefer this over the previous version since it makes selecting/removing a lot more obvious than the opacity change on a white background.
if (erroredOut) { | ||
return ( | ||
<div className="github-repos github-repos-errored"> | ||
An error occurred. Please check your browser console and email |
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 adds error handling to the page, in case the asynchronous route times out or some other error occurs.
constructor(props) { | ||
super(props); | ||
const { selected } = this.props; | ||
this.state = { selected }; |
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 originally had this.state = this.props.selected
but the linter threw an error saying I needed to use destructuring assignment. I see that it's useful but it seems more verbose to me (literally two lines versus one). Also, I had to do change it everywhere else where I was using this.props.something
, so personally think we should change that rule.
resources :follows, only: [:create] | ||
resources :github_repos, only: [:index] do | ||
collection do | ||
post "/update_or_create", to: "github_repos#update_or_create" |
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.
Using one route for updating and creating since we have in the Github Repo model/table github_repo.rb
a find_or_create
method.
I added some basic tests for the components. @benhalpern @maestromac want to take a look? |
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 is so well done.
What type of PR is this? (check all applicable)
Description
/settings/integrations
is now loaded in asynchronously via Preact/WebpackRelated Tickets & Documents
Resolves #830 and resolves #822
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?