Jeremy/eng 997b current upcoming #222

jerminatorhits · 2019-02-28T21:07:37Z

Description

Why did you write this code?
display current and upcoming trips
port over the contact host modal to bootstrap
port over the cancelBooking alert/modal to bootstrap

How to Test

Visit '/work/trips'
Click 'upcoming' and 'current' to see respective bookings
Send message via 'Contact Host' to host successfully
Successfully cancel a booking using 'Cancel Booking'

Which devices did you test on?

REVIEWERS:

Check against these principles:

High level

Does this code need to be written?
What are the alternatives?
Will this implementation become a support issue?
How much error margin does this solution have?

Code

Does the code follow industry standards?
JS: https://github.com/airbnb/javascript
React: https://github.com/airbnb/javascript/tree/master/react
https://github.com/vasanthk/react-bits
Documentation headers: http://usejsdoc.org/index.html
Is there duplicated code? Can it be refactored into a shared method?
Is the code consistent with our project?
Are there unit tests? Do they test the states?
Is the person refactoring another developer's code? If possible, did the original developer approve?

Variables/Naming:

Would the variable type led to future edge cases?
Are the variable naming clear? Would the value contain something other than what the name describes.

Security

Can this be hacked or abused by the user?

Further Work

Will this have future work?

Learnings

Did you learn anything here you want to share with the team?

jerminatorhits · 2019-03-01T07:18:31Z

Note: Using the same cards for all 4 tabs. Weird thing was that I was able to cancel a booking on the 'Cancelled/Rejected' tab.

Next PR will do the following:

remove 'cancel booking' button
add the red status text for cancelled/reject cards
fix photo image to fit different screens and card sizes, without quality loss.

woeltjen · 2019-03-01T19:50:36Z

src/components/work/CancelBookingModal.tsx/CancelBookingModal.tsx

+      <ModalHeader>Cancel Booking</ModalHeader>
+      <ModalBody>
+        <h6>Are you sure you want to cancel this booking?</h6>
+        {booking && <h6>{`Booking: ${booking.id}`}</h6>}


This looks equivalent to:

<h6>Booking: {booking.id}</h6>

(booking is non-optional in Props so the check has been handled at compile-time, and there's some extra syntax around the templated part)

oo makes sense. thanks!

woeltjen · 2019-03-01T19:52:23Z

src/components/work/CancelBookingModal.tsx/CancelBookingModal.tsx

+      </ModalBody>
+      <ModalFooter>
+        <Button color="secondary" disabled={isSubmitting} onClick={() => handleModalAction()}>Back</Button>{' '}
+        <Button color="danger" disabled={isSubmitting} onClick={() => handleCancelBooking()}>Yes, Cancel Booking</Button>


Both onClick handlers are no-argument functions so you should be able to pass them in directly:

onClick={handleModalAction} onClick={handleCancelBooking}

brilliant. will keep in mind in the future!

woeltjen · 2019-03-01T19:57:21Z

src/components/work/CancelBookingModal.tsx/CancelBookingModal.tsx

+  );
+
+  function handleCancelBooking() {
+    if (!booking) return;


This check is also unnecessary (handled at compile-time by enforcing Props interface)

👍 the node backend in me must've gotten nervous =x

woeltjen · 2019-03-01T20:00:53Z

src/components/work/CancelBookingModal.tsx/CancelBookingModal.tsx

+interface Props {
+  booking: Booking;
+  cancelBooking: (booking: Booking) => Promise<Booking>;
+  handleModalAction: () => void;


For consistency with React naming conventions, I'd expect this to be onModalAction at the interface level and handleModalAction at the implementation level, for usage like:

<CancelBookingModal onModalAction={handleModalAction} etc... />

that was bugging me in the back of my head. I'll try to remember or refer back to this in the future. tyty!

woeltjen · 2019-03-01T20:01:51Z

src/components/work/ContactHostFormModal/ContactHostFormModal.tsx

+  message: Yup.string().required('Please fill out the message field.'),
+});
+
+function ContactHostForm({ booking, contactUser, handleModalAction }: Props) {


Oh yay, this will come in handy for the Contact Host button on the listing page

woeltjen · 2019-03-01T20:02:05Z

src/components/work/ContactHostFormModal/ContactHostFormModal.tsx

+interface Props {
+  contactUser: (input: ContactUserInput) => Promise<EmailResponse>;
+  booking: Booking;
+  handleModalAction: () => void;


As above, I'd expect this to be named onModalAction

woeltjen · 2019-03-01T20:13:27Z

src/components/work/TripCard/TripCard.tsx

+        <CardTitle className="h5 font-weight-normal mb-3">{booking.listing.title}</CardTitle>
+        <CardSubtitle className="small mb-3">
+          {addressLine1 && formatAddress(addressLine1, addressLine2, city, state, country, postalCode)}
+          {!addressLine1 && formatGeolocationAddress({ lat, lng, city, country })}


Small change, but I'd do this as:

{addressLine1 ? formatAddress(...) : formatGeolocationAddress(...)}

Rationale is maintainability; the logic for the two checks is necessarily complementary (we want exactly one of these formatted outputs to be displayed) but separating them allows them to diverge (e.g if we change addressLine1 to (addressLine1 || addressLine2) on one line but not the next)

Also, I may be misremembering, but I think we added a fullAddress field to listings on the backend that handles this formatting ahead of time, so you might just be able to say booking.listing.fullAddress here.

Yep, backend defines fullAddress as:

fullAddress: { type: DataTypes.VIRTUAL, get: function () { const { addressLine1, addressLine2, city, state, country, postalCode, lat, lng } = this; return addressLine1 ? formatAddress(addressLine1, addressLine2, city, state, country, postalCode) : formatGeolocationAddress({ lat, lng, city, country }); } }

May need to update GraphQL queries to expose it tho; in the GraphQL booking end point there is a Listing type defined which does not include this.

interesting. definitely on board with the maintainability factor (leaves smaller margin of error as stated)

As for the fullAddress, I think some cards should only show city, state, country, etc. (omitting street address since it shouldn't be shown to unapproved bookings). Would it be better practice to handle that in the backend?

actually lemme smoke test this

As for the fullAddress, I think some cards should only show city, state, country, etc. (omitting street address since it shouldn't be shown to unapproved bookings). Would it be better practice to handle that in the backend?

Ah, you're right; fullAddress (as currently defined) should only be exposed for approved bookings. Sounds like we want something like a displayAddress that takes into account the privileges of the user doing the viewing? In any case, that's getting out of scope for this issue.

ahh makes sense. Will put in the fix for a ternary at the very least

tc · 2019-03-02T00:28:35Z

The Contact Host should be an a tag or button so it gets a natural mouse cursor
Same with Cancel Trip.
right now, it's a text selection.

tc · 2019-03-02T00:30:37Z

src/pages/trips/Trips.tsx

+            </Row>
+            <Row>
+              <a href='/work'>
+                <Button>Book a Home Today!</Button>


you can remove the button tag and this can be <a className="btn"> so the cursor is correctly registered.

jerminatorhits · 2019-03-02T01:39:08Z

seems like <a> tag only provides the pointer when an href is declared (which won't work for the modal).

Button has a lot of intrinsic padding as well

Made a custom css class after doing some research on a pretty back-and-forth discussion: twbs/bootstrap#23709

.c-pointer {
 cursor: pointer;
}

tc · 2019-03-03T02:51:29Z

@jerminatorhits curious, could you add href="#" to the a tag? I think the javascript does a preventDefault() to overridden the call.

tc · 2019-03-03T02:54:42Z

src/components/work/TripCard/TripCard.tsx

+        <Row className="align-items-center">
+          <Col
+            xs="4"
+            onClick={() => handleModalAction(ModalType.CONTACT_HOST)}


could this be onClick={handleOpenContactHostModal} and the method call handleModalAction(ModalType.CONTACT_HOST) ?

could do. since there's also a CANCEL_BOOKING modal, that would require a handleOpenCancelBookingModal to be passed into this component as well. If it's conventionally better to use specific handle names, I'm all for it.

i think using a method can ensure better enforcement vs a parameter.

i see. so the component doesn't have to know about the enum. will change

jerminatorhits · 2019-03-04T08:06:31Z

@tc put an href="#" on the a tag and it has the pointer but goes to home page.

tc · 2019-03-04T18:17:17Z

@tc put an href="#" on the a tag and it has the pointer but goes to home page.
does the onClick listener have a e.preventDefault or e.stopPropagation ?

jerminatorhits · 2019-03-04T18:30:13Z

@tc <a href="#" onClick={(event: React.MouseEvent<HTMLElement>) => event.preventDefault()}>

includes the pointer and opens the modal. Interesting!

jerminatorhits added 3 commits February 27, 2019 15:46

building card WIP

40d039a

get basic card layout to show

de517e9

get card spacing to be even

e29479f

jerminatorhits requested review from tc, kcvan and woeltjen February 28, 2019 21:07

jerminatorhits added 4 commits February 28, 2019 15:28

modal WIP

198d8ba

cancel booking working

2e65a28

finish contact host modal

1e95308

cancel booking modal flow working

4e30cac

jerminatorhits changed the title ~~Jeremy/eng 997b current upcoming WIP~~ Jeremy/eng 997b current upcoming Mar 1, 2019

jerminatorhits added 2 commits February 28, 2019 21:56

merge

eec8176

condense code

58cd299

jerminatorhits mentioned this pull request Mar 1, 2019

Jeremy/eng 997c past cancelled #227

Merged

9 tasks

woeltjen reviewed Mar 1, 2019

View reviewed changes

jerminatorhits added 3 commits March 1, 2019 11:52

remove extra templating

21144be

remove unnecessary arrow func

8b558bb

set currency before/same time as isSubmitting Loading renders

18296c1

woeltjen reviewed Mar 1, 2019

View reviewed changes

remove unnecessary check

5992a51

woeltjen reviewed Mar 1, 2019

View reviewed changes

jerminatorhits added 3 commits March 1, 2019 12:02

remove unneeded state

ec1a363

Merge branch 'master' into jeremy/ENG-997b-current-upcoming

d223c69

onModalAction naming

2d83b97

woeltjen reviewed Mar 1, 2019

View reviewed changes

format address ternary

f3c054b

Merge branch 'master' into jeremy/ENG-997b-current-upcoming

97114a6

tc reviewed Mar 2, 2019

View reviewed changes

remove button, add classNames to a tag

c0caa2b

add c-pointer class

62c2eb9

tc reviewed Mar 3, 2019

View reviewed changes

use explicit functions for modal

06ee2ae

jerminatorhits added 3 commits March 4, 2019 10:32

swap pointer class with a tag + prevent default

ee49f08

add click anywhere to close modal

a822732

localize event.preventDefault and openModal methods

1eb140d

tc approved these changes Mar 4, 2019

View reviewed changes

jerminatorhits merged commit 7f994de into master Mar 4, 2019

jerminatorhits deleted the jeremy/ENG-997b-current-upcoming branch March 4, 2019 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jeremy/eng 997b current upcoming #222

Jeremy/eng 997b current upcoming #222

jerminatorhits commented Feb 28, 2019

jerminatorhits commented Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

jerminatorhits Mar 1, 2019

woeltjen Mar 1, 2019

jerminatorhits Mar 1, 2019

tc commented Mar 2, 2019 •

edited

Loading

tc Mar 2, 2019 •

edited

Loading

jerminatorhits Mar 2, 2019

jerminatorhits commented Mar 2, 2019

tc commented Mar 3, 2019

tc Mar 3, 2019

jerminatorhits Mar 4, 2019

tc Mar 4, 2019

jerminatorhits Mar 4, 2019

jerminatorhits commented Mar 4, 2019

tc commented Mar 4, 2019

jerminatorhits commented Mar 4, 2019

Jeremy/eng 997b current upcoming #222

Jeremy/eng 997b current upcoming #222

Conversation

jerminatorhits commented Feb 28, 2019

Description

How to Test

REVIEWERS:

High level

Code

Variables/Naming:

Security

Further Work

Learnings

jerminatorhits commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tc commented Mar 2, 2019 • edited Loading

tc Mar 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerminatorhits commented Mar 2, 2019

tc commented Mar 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerminatorhits commented Mar 4, 2019

tc commented Mar 4, 2019

jerminatorhits commented Mar 4, 2019

tc commented Mar 2, 2019 •

edited

Loading

tc Mar 2, 2019 •

edited

Loading