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

feat: data collection for search #207

Merged
merged 12 commits into from
Jun 24, 2020
Merged

Conversation

JasonChong96
Copy link
Contributor

@JasonChong96 JasonChong96 commented Jun 18, 2020

Problem

Before link search can be fully implemented, further collection of data from link owners is required.

#181

Solution

This PR implements phase 0 of the implementation for link search. UI Fields and database columns are added for contact information and description for each link.

Improvements:

  • Updated URL drawer margins to fit the design more
  • Use of negative margins in DrawerTextField have been replaced by having helper text in absolute position. The negative margins made it hard to debug element positioning in browser debuggers and calculations of margins needed for subsequent elements were also confusing.

Before & After Screenshots

AFTER:
Desktop
Mobile

Deploy Notes

The new migration script should be run before deployment. The new columns will be nullable and compatible with both the new and old code.

TODO:

  • Create sql migration file
  • Update url retrieval endpoint to return new fields
  • Add new fields in edit url UI (Desktop)
  • Add new fields in edit url UI (Mobile)
  • Update call to url edit API
  • Update url edit endpoint
  • Add validation for contact email

@JasonChong96 JasonChong96 marked this pull request as draft June 18, 2020 12:13
@JasonChong96 JasonChong96 force-pushed the search-data-collection branch from 8cd43b6 to 0cd76e2 Compare June 22, 2020 07:52
@JasonChong96 JasonChong96 marked this pull request as ready for review June 22, 2020 08:37
Comment on lines 93 to 98
allowNull: false,
validate: {
isEmail: true,
isLowercase: true,
is: emailValidator.makeRe(),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't there an inconsistency, if the default value populated by the SQL migration is an empty string, but here we are insisting on allowNull: false and isEmail: true?

would it be easier to simply allow null values?

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

it's dangerous to backfill empty strings in the DB, yet insist on non-nullable email validation in the ORM.

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

lgtm

@liangyuanruo liangyuanruo merged commit 527037b into develop Jun 24, 2020
@liangyuanruo liangyuanruo deleted the search-data-collection branch June 24, 2020 06:20
@yong-jie yong-jie mentioned this pull request Apr 1, 2022
24 tasks
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.

2 participants