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

Airtable refactor, bug fixes, Auth overhaul #48

Merged
merged 45 commits into from
Apr 3, 2020
Merged

Conversation

annieyro
Copy link
Collaborator

@annieyro annieyro commented Mar 26, 2020

What's new in this PR

Refactoring our Airtable code in this repo to use airtable-schema-generator (now that it's been updated to 1.3.3 with some nice features!).

REQUIRES .env.development/production to be changed from AIRTABLE_API_KEY to REACT_APP_AIRTABLE_API_KEY

...for compatibility for v1.3.3

Has the same Airtable schema updates as calblueprint/dccentralkitchen-clerks#29; Points History screen will not load without transaction columns being changed to Total Sale.

  • Removed code in common.js since BASE will no longer be used

Map

  • Bug: initial store showed the incorrect products
  • Edge case: If no products are linked to a store, the key productIds will not exist on the store object, so special handling is required

Navigation

  • Cleanup: refactored AppNavigator by extracting DrawerContent as a class, and moving AuthStack to StackNavigators

News

  • Bug: NewsScreen displayed Created date, not Post Date

Rewards

  • Deprecated: removed pull-to-refresh implementation to declutter the file. We can add it back from a different trigger if necessary. Removed deprecated props which were not being used in RewardsHome and PointsHistory components.
  • Cleanup: Made Overline components normally-cased (since the styled component uses text-transform)
  • Cleanup: Added isLoading to DrawerContent and RewardsScreen. This is probably the most crucial place to add a loading screen (@ace) imo; it's noticeably laggy here.
  • Cleanup: Add Rewards.js to constants so things aren't hardcoded.

Auth

Integrated @tommypoa 's changes from material-ui branch

SignUp

  • Fixed SignUp's duplicate customer check to actually work, but no design exists for displaying the error right now (@12aschen)
  • Replaced the name, nameError etc in this.state to use an object similar to indicators.
  • Changed behavior of handleErrorState to update error messages from validate.js instead of being hardcoded into AuthField
  • Added an error message and error state for name
  • Made onTextChange hooks async so that error handling would happen after values updated in state; and handled permissions
  • Note that onBlur is still required for the edge case of user tapping in then tapping out
  • Added margin to the SignUp button

Login

  • Added error message and no error message but error indicators for that state.
  • Changed Login to LogIn/Log In where necessary (and Logout in the drawer)
  • closes Change Login and Logout to Log In and Log Out #69
  • Hardcoded the size of TextFieldContainer (losing transition animations to error states) to do so.

Relevant Links

Related PRs

Waiting on @tommypoa for some last updates to auth

How to review

Nothing should really change appearance-wise or functionality-wise for anything except the Signup/Login flow! But everything should still work (^:

Next steps

Would be nice to change userId to customerId for consistency between the repos.

Tests Performed, Edge Cases

Signup: https://www.loom.com/share/896a8bb1e5ba442281908122fc35e298

Login: https://www.loom.com/share/947060a3784242bc8e79f91b24c8c386

Screenshots

CC: @wangannie

tommypoa and others added 14 commits March 23, 2020 00:36
 - Deprecated: removed pull-to-refresh implementation to declutter the file. We can add it back from a different trigger if necessary. Removed deprecated props which were not being used in `RewardsHome` and `PointsHistory` components.
- Cleanup: Made `Overline` components normally-cased (since the styled component uses `text-transform`)
- Cleanup: Added `isLoading` to `DrawerContent` and `RewardsScreen`. This is probably the most crucial place to add a loading screen (@ace) imo; it's noticeably laggy here.
prettier.config.js Outdated Show resolved Hide resolved
@annieyro annieyro marked this pull request as ready for review April 1, 2020 04:50
@annieyro annieyro requested a review from aivantg April 1, 2020 04:50
Copy link
Member

@wangannie wangannie left a comment

Choose a reason for hiding this comment

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

👏 👏 Wow very cool stuff!

I just noted a couple details and bugs. Also, this might have nothing to do with this, but overall I've noticed that it's loading substantially slower, particularly upon logging in and loading the Rewards screen/points history. It definitely feels like a lot more time spent staring at a white screen...is that something we can do anything about, or maybe we just need to prioritize adding loading screens?

.env.example Outdated Show resolved Hide resolved
components/rewards/RewardsHome.js Outdated Show resolved Hide resolved
components/rewards/RewardsHome.js Show resolved Hide resolved
constants/Rewards.js Show resolved Hide resolved
components/rewards/RewardsHome.js Outdated Show resolved Hide resolved
lib/mapUtils.js Outdated Show resolved Hide resolved
navigation/StackNavigators.js Outdated Show resolved Hide resolved
screens/auth/SignUpScreen.js Outdated Show resolved Hide resolved
screens/auth/SignUpScreen.js Outdated Show resolved Hide resolved
// In case of database malformation, may return more than one record
// TODO this message is a design edge case
error =
'Database error: more than one customer found with this login information. Please report an issue so we can fix it for you!';
Copy link
Member

Choose a reason for hiding this comment

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

Well the 'report issue' link only shows in the hamburger menu which requires you to be logged in 😅 @12aschen

@wangannie wangannie requested a review from kennethlien April 2, 2020 17:54
This was referenced Apr 2, 2020
@annieyro annieyro force-pushed the annie/update-airtable branch from 5afadac to 55a5608 Compare April 2, 2020 21:10
@annieyro
Copy link
Collaborator Author

annieyro commented Apr 2, 2020

Realized that we shouldn't have been keeping permissions in state at all, and the unnecessary async/await pattern was making things messy/slow, so updated that ;; eliminated some code in SignUp and LogIn (':

@annieyro
Copy link
Collaborator Author

annieyro commented Apr 3, 2020

Copy link
Contributor

@kennethlien kennethlien left a comment

Choose a reason for hiding this comment

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

This is crazy bruh ur a goat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Login and Logout to Log In and Log Out
4 participants