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.

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

Added help text to loading page #197

Merged
merged 5 commits into from
Feb 21, 2020
Merged

Added help text to loading page #197

merged 5 commits into from
Feb 21, 2020

Conversation

rzlien
Copy link
Contributor

@rzlien rzlien commented Feb 8, 2020

Added help text to loading page if loading takes more than 2 seconds. Allows users to submit issues to Github through a link on the loading page.

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Hi Rachel,

Great work on your first PR! I know it was a big confusing navigating React/props/state/components, but I'm glad you were able to figure it out.

From a functionality perspective, this works exactly as intended, great work! I do have a few pointers on code consistency (using SASS instead of the style tag) and some nitpicky styling stuff, but if you're able to implement that (which shouldn't take too long) I think this looks good to merge. Solid work again, and let me know if you have any questions!

(as a quick side note, we would eventually want to write a test that confirms this behaviour. I'll either assign you another ticket to work on that if you're interested, or put you on another editor feature if you'd prefer that instead)

src/components/common/LoadingPage.js Show resolved Hide resolved
src/components/common/LoadingPage.js Outdated Show resolved Hide resolved
@mattxwang mattxwang added the feature New feature or request label Feb 12, 2020
Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Awesome Rachel, thank you for implementing this ticket! Great first issue done, and on to the next one 😄 LGTM!

@mattxwang mattxwang merged commit 3dd58ef into uclaacm:master Feb 21, 2020
mattxwang added a commit that referenced this pull request Feb 22, 2020
This is a quick follow up to @rzlien's PR (#197) adding a help text timer to `<Loading>`. It does three things:

1. renames the prop `showText` to `showHelpText`, and makes it inherit the value from an optional prop
2. uses `GH_REPO_NAME` constant instead of hard link for repo
3. adds tests to check for `showHelpText` prop validity and to check that the timer works (i.e. on mount, the help text is not there, and then after 2s, the help text shows up)

Not super happy with how the timer test is written, since I wanted to use Jest's [mock timer calls](https://jestjs.io/docs/en/timer-mocks) - but I wasn't able to get them to work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants