Switching to Practitioner Object. #5 #8

dsych · 2018-10-21T17:20:50Z

Things to consider:

Search query parameters for mailingAddress, phoneNumber, practiceAddress see.
Not using mailingAddressLine2 and practiceAddressLine2. We might want to incorporate them, or just drop. At any rate, they are private for now.

Closes #5

vercel · 2018-10-21T17:20:53Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

src/components/counselor-table.tsx

src/app.tsx

Redmega

Really awesome work @dsych! Thanks for the contribution!

Search query parameters for mailingAddress, phoneNumber, practiceAddress see.

We'll need to add special handlers for those searches in the server. Searching will likely be overhauled when the new "Contact Card" UI gets created, so I wouldn't worry about this yet.

Not using mailingAddressLine2 and practiceAddressLine2. We might want to incorporate them, or just drop. At any rate, they are private for now.

Hmm... I do think they should be added to the address, right after *AddressLine1. I'm not actually sure how many of the entries actually have an AddressLine2, but usually it's used for things like apartment number or suite number, so we shouldn't just drop it.

This is a great start to revamping the data structure. The next step would be to do the object creation server side. Then our class can be simplified to

constructor(data) {
  Object.keys(data).forEach(key => { this[key] = data[key] });
}

If you'd like to take that on as well, let me know, otherwise we can merge this and create a new issue for that.

src/components/counselor-table.tsx

src/app.tsx

src/components/practitioner.ts

Redmega · 2018-10-21T17:48:10Z

Deployment looks great! I can't wait to lose this huge table

https://fl-licensure-jsvrppddik.now.sh/

dsych · 2018-10-21T19:40:16Z

@Redmega I think we should split UI and server into 2 PRs. Having said that, I am not opposed to continue working on this server side as well.

Redmega · 2018-10-22T13:40:14Z

src/components/practitioner.ts

@@ -31,7 +31,8 @@ export default class Practitioner {
    private mailingAddressZIP: string;
    private mailingAddressArea: string;
    get mailingAddress(): string {
-        return `${this.mailingAddressLine1}, ${this.mailingAddressCity} ${this.mailingAddressState}, ${this.mailingAddressZIP} ${this.mailingAddressArea}`;
+        return `Line 1: ${this.mailingAddressLine1}, ${this.mailingAddressCity} ${this.mailingAddressState}, ${this.mailingAddressZIP} ${this.mailingAddressArea}.
+        ${this.mailingAddressLine2 ? " Line 2: " + this.mailingAddressLine2 : ""}`;


123 Test Lane # Line 1 APT 42 # Line 2 Test City, Florida 33111 # Rest of the stuff

nit Just a nitpick, an address generally looks like this.

Not a nitpick, however, is that the mailingAddressArea is the area code for the number (Which, for some reason, is tied to the mailing address!? Don't ask me why the FL gov decided to do it that way...) That should definitely be part of the get phoneNumber().

I can't really break up the line 1 and 2 with a newline without either returning html (don't like this option at all) or an array that is later joined with <br>.

UPDATE
We could do something like Line 1: ... (Line 2: ...), ...

Just to clarify, you want the phone number to be (areaCode) phoneNum Ext - extension? I am not from US, so I don't know.

Hmm... I don't like the idea of returning html either. What we can do is maybe return either just an array like [line1, line2, line3] and when we render address do:

// Will need some styling `address span { display: block; }` <address>{ address.map(line => <span>{line}</span> ) } </address>

Alternatively, we can return an object with the different address fields and pass that object to an Address component when we render. That way we aren't returning html, we're just passing the address object to a react component which will give us our jsx. I actually like this approach more, but the previous one will probably be ever so slightly quicker to implement. I leave it up to you to decide which approach to take 😄

Just to clarify, you want the phone number to be (areaCode) phoneNum Ext - extension? I am not from US, so I don't know.

That's right! Sorry, didn't realize Canadian numbers were different!

A phone number should ideally look like (123) 456-7890 x999 where (123) is the area code, 456-7890 is the phone number, and 999 is the extension (denoted by the x)

I think the numbers annoyingly come with some formatting already, but I'm pretty sure the area code is there without parens, so we can at least include that one. If you just did

get phoneNumber() { return `(${areaCode}) ${phoneNum} - ${extension}` }

that would be good enough ™️ for our purposes.

Redmega · 2018-10-22T14:11:14Z

src/components/practitioner.ts

@@ -41,7 +42,8 @@ export default class Practitioner {
    private practiceAddressState: string;
    private practiceAddressZIP: string;
    get practiceAddress(): string {
-        return `${this.practiceAddressLine1}, ${this.practiceAddressCity} ${this.practiceAddressState}, ${this.practiceAddressZIP}`;
+        return `Line 1: ${this.practiceAddressLine1}, ${this.practiceAddressCity} ${this.practiceAddressState}, ${this.practiceAddressZIP}.
+        ${this.practiceAddressLine2 ? " Line 2: " + this.practiceAddressLine2 : ""}`;


Same as the above, we should try to follow the common address formatting

Redmega · 2018-10-22T14:12:36Z

Sorry, I wanted to write these comments last night but Github happened to be down all of a sudden.

Yeah, I'm fine with having the server stuff be in a separate PR. I''ll create the issue at some point tonight.

dsych · 2018-10-23T00:30:00Z

@Redmega I've made some substantial changes, mainly following what you proposed for address and phone number by implementing a custom Address and Phone interfaces that are rendered by AddressComponent and PhoneComponent, respectively.
Also had to increase the cell height to fit the new address.

Redmega · 2018-10-23T01:09:16Z

src/components/practitioner.ts

@@ -1,5 +1,5 @@

-export default class Practitioner {
+export class Practitioner {


For future reference, you can still export default and also export named stuff below. You'd import like so:

import Practitioner, { instanceOfPhone, instanceOfAddress } from 'blah'

But that's not a blocker to getting this merged! ✔️

dsych · 2018-10-23T01:28:55Z

Ok, merge when you are ready.

switching to objects

2cd366a

dsych mentioned this pull request Oct 21, 2018

Process data in order to implement category based searching #5

Closed

dsych commented Oct 21, 2018

View reviewed changes

src/components/counselor-table.tsx Show resolved Hide resolved

src/app.tsx Outdated Show resolved Hide resolved

Redmega suggested changes Oct 21, 2018

View reviewed changes

src/components/counselor-table.tsx Outdated Show resolved Hide resolved

src/app.tsx Outdated Show resolved Hide resolved

src/components/practitioner.ts Outdated Show resolved Hide resolved

mergin line 1 and 2. minor fixes.

5c03773

dsych force-pushed the practitioner branch from 4424c7d to 5c03773 Compare October 22, 2018 01:12

Redmega suggested changes Oct 22, 2018

View reviewed changes

Redmega mentioned this pull request Oct 23, 2018

The Server should separate the counselor entries into objects before saving to the database. #9

Closed

switching to address and phone objects with components.

f970a66

Redmega approved these changes Oct 23, 2018

View reviewed changes

Redmega merged commit 6caea76 into Redmega:master Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switching to Practitioner Object. #5 #8

Switching to Practitioner Object. #5 #8

dsych commented Oct 21, 2018 •

edited by Redmega

Loading

vercel bot commented Oct 21, 2018

Redmega left a comment •

edited

Loading

Redmega commented Oct 21, 2018

dsych commented Oct 21, 2018

Redmega Oct 22, 2018 •

edited

Loading

dsych Oct 22, 2018 •

edited

Loading

dsych Oct 22, 2018 •

edited

Loading

Redmega Oct 22, 2018 •

edited

Loading

Redmega Oct 22, 2018

Redmega commented Oct 22, 2018

dsych commented Oct 23, 2018 •

edited

Loading

Redmega Oct 23, 2018

dsych commented Oct 23, 2018

		@@ -1,5 +1,5 @@

		export default class Practitioner {
		export class Practitioner {

Switching to Practitioner Object. #5 #8

Switching to Practitioner Object. #5 #8

Conversation

dsych commented Oct 21, 2018 • edited by Redmega Loading

vercel bot commented Oct 21, 2018

Redmega left a comment • edited Loading

Choose a reason for hiding this comment

Redmega commented Oct 21, 2018

dsych commented Oct 21, 2018

Redmega Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

dsych Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

dsych Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Redmega Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Redmega Oct 22, 2018

Choose a reason for hiding this comment

Redmega commented Oct 22, 2018

dsych commented Oct 23, 2018 • edited Loading

Redmega Oct 23, 2018

Choose a reason for hiding this comment

dsych commented Oct 23, 2018

dsych commented Oct 21, 2018 •

edited by Redmega

Loading

Redmega left a comment •

edited

Loading

Redmega Oct 22, 2018 •

edited

Loading

dsych Oct 22, 2018 •

edited

Loading

dsych Oct 22, 2018 •

edited

Loading

Redmega Oct 22, 2018 •

edited

Loading

dsych commented Oct 23, 2018 •

edited

Loading