Skip to content
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.

Sign up for GitHub

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

Jump to bottom

API limit exceed: Show modal #184

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

API limit exceed: Show modal #184

wants to merge 2 commits into from

Conversation

123vivekr
Copy link
Member

@123vivekr 123vivekr commented Jan 17, 2019 •
edited
Loading

Shows the error in a user friendly way.

Closes #127

@wisn
Copy link
Member

wisn commented Jan 17, 2019

@jayvdb @blazeu which JavaScript style guide we use for this repository? Is it Airbnb? I would like to review this pull request.

@li-boxuan
Copy link
Member

We didn't specify or regulate a javascript style, but airbnb style seems to be the thing we were actually practicing.

wisn
wisn suggested changes Jan 17, 2019
View reviewed changes
Copy link
Member

@wisn wisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff looks weird. Anyway, please follow Airbnb JavaScript style guide. We use two whitespaces as the indentation. Don't change it into four whitespaces.

Also, when you are at it is, edit all const {something} into const { something } would be great. I would do extensive review after this.

@123vivekr
Copy link
Member Author

123vivekr commented Jan 18, 2019 •
edited
Loading

@wisn Thank you for reviewing my Pull Request. I will change my code to follow the airbnb style guide.

This is the modal.
20190117_100548

I'm having problem with the state to toggle the visibility of the modal. I am not able to update state within static defaultProps object. Please guide me on what changes I have to make here.
Consequently, the Ok button does not do anything.

PS: Please ignore the dark theme

@gitmate-bot gitmate-bot added size/S and removed size/M labels Jan 23, 2019
@123vivekr
Copy link
Member Author

I've made the requested changes - changed space indentation to 2 spaces and changed
const {identifier} to const { identifier }

@123vivekr
Copy link
Member Author

@wisn Please review my PR

@wisn
Copy link
Member

wisn commented Jan 31, 2019

@123vivekr Does the modal works now? I don't know why the deploy preview always failed

@123vivekr
Copy link
Member Author

@wisn I changed the props to functions and gave a show variable in the state. But the onResolve and onError functions are called (which in turn calls setState) multiple times and causes an infinite loop.

@123vivekr
Copy link
Member Author

123vivekr commented Feb 2, 2019 •
edited
Loading

@wisn Please help figure out the cause of multiple calls to onError and onResolve.

@jayvdb
Copy link
Member

jayvdb commented Feb 3, 2019

@gitmate-bot rebase

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@gitmate-bot
Copy link

Automated rebase with GitMate.io was successful! 🎉

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 7c20ed10-278f-11e9-81eb-21ccd9c7497f

jayvdb
jayvdb requested changes Feb 3, 2019
View reviewed changes
src/components/loadable.jsx Outdated Show resolved Hide resolved
src/components/board.jsx Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: d0613370-27d1-11e9-81eb-21ccd9c7497f

@gitmate-bot gitmate-bot added size/M and removed size/S labels Feb 3, 2019
@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: d6fdf350-2c25-11e9-b3d3-256864349aa7

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: fc846d20-2c25-11e9-b3d3-256864349aa7

@123vivekr
Copy link
Member Author

20190210_211950
This is the final modal.

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: fb35e080-2d4b-11e9-bee6-ebd77371f46a

@KVGarg
Copy link

KVGarg commented Feb 10, 2019 •
edited
Loading

@123vivekr I have no knowledge regarding react language, but regarding coding standards I have one question. Since, the code is getting repeated at 1 and 2. So, can we create a single function for it that contains the repeated code and it accepts the args/kwargs for the content which is getting changed i.e. of paragraph, header etc.
If that is possible it will be much good.

@123vivekr
Copy link
Member Author

123vivekr commented Feb 10, 2019 •
edited
Loading

@KVGarg Requested changes have been made. :)
Thank you for the suggestion

@KVGarg
Copy link

KVGarg commented Feb 10, 2019

Also, haven't notices the number of commits before, sorry.
Last change, Please squash your commits.
After that 💯

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: a2b85b70-2d52-11e9-bee6-ebd77371f46a

@123vivekr
Copy link
Member Author

123vivekr commented Feb 10, 2019 •
edited
Loading

@KVGarg The commits are for different changes in different files. I don't think they need to be squashed.
Thank you for reviewing.

jayvdb
jayvdb requested changes Feb 11, 2019
View reviewed changes
src/components/loadable.jsx Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: e2ea4b00-2e69-11e9-93d2-519d0ef52680

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 3bba9f70-2e6d-11e9-93d2-519d0ef52680

@123vivekr 123vivekr force-pushed the api-limit-exceed branch 2 times, most recently from aa72355 to 8627c41 Compare February 12, 2019 02:27
@123vivekr
loadable.jsx: Show modal on API limit exceed
dd2b661 
This shows a modal with error message on exceeding API limit.

Closes coala#127
@123vivekr
board.jsx: Make error message user friendly
183b7f4 
Closes coala#127
@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 183d46a0-2e6e-11e9-93d2-519d0ef52680

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 6a3eeda0-2e6e-11e9-93d2-519d0ef52680

@TravisBuddy
Copy link

Travis tests have failed

Hey @123vivekr,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: b5167960-2e6e-11e9-93d2-519d0ef52680

KVGarg
KVGarg suggested changes Mar 18, 2019
View reviewed changes
Copy link

@KVGarg KVGarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Dialog box not appearing when API limit got exceeded. Can you take a look why is this happening?
Browsed page url

@RJ722 RJ722 mentioned this pull request Sep 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KVGarg KVGarg KVGarg requested changes

@wisn wisn wisn requested changes

@jayvdb jayvdb jayvdb requested changes

Assignees

@123vivekr 123vivekr

Labels
enhancement process/pending review size/XS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@123vivekr @wisn @li-boxuan @jayvdb @gitmate-bot @TravisBuddy @KVGarg