Skip to content

listing-form-sidebar-help (visit https://dev03.beenest.io/host/listings/87134/listing_info)#113

Merged
jerminatorhits merged 40 commits intomasterfrom
jeremy/ENG-920-listing-helper-section
Feb 7, 2019
Merged

listing-form-sidebar-help (visit https://dev03.beenest.io/host/listings/87134/listing_info)#113
jerminatorhits merged 40 commits intomasterfrom
jeremy/ENG-920-listing-helper-section

Conversation

@jerminatorhits
Copy link
Contributor

@jerminatorhits jerminatorhits commented Jan 31, 2019

Description

Why did you write this code?
Add text helpers to the listing form side bar. Mostly just have the scaffolding down and can make a separate PR for full detailed descriptions.

How to Test

  • Visit host/listings/:id/edit
  • Click various fields and tabs for side info

Which devices did you test on?

  • Chrome on Mac
  • Chrome on PC
  • Firefox on Mac
  • Firefox on PC
  • Safari iPhone
  • Chrome Android

@jerminatorhits jerminatorhits changed the title listing-form-sidebar-help WIP listing-form-sidebar-help (WIP visit https://dev03.beenest.io/host/listings/87134/listing_info) Feb 1, 2019
pricing_availability: 'pricing availability header',
checkin_details: 'checkin details header',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking into consolidating properties based on each crumb. seems like there are a lot of declared objects with the crumb as the key.


const Title = () => (
<div>
<p>This is ListingName Info</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if these should e more use-friendly since they're consumer facing? Maybe something like This is the listing title. Same for the ones below this one:

This is the description for the listing.
This is the country the listing is located in
This is the listing's cover photo
..etc

Copy link
Contributor Author

@jerminatorhits jerminatorhits Feb 1, 2019

Choose a reason for hiding this comment

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

oh these were just placeholders haha. The scaffolding was more of the goal of this PR, prob could have been more clear soz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated match the listing doc: https://docs.google.com/document/d/1w1RCVl9lIHqGA5UOVk3B0zqgRG3qSDBlfsQP_iiF0hg/edit#

some fields don't have explanations -- left them blank for now.


const AsideHeaders: AsideHeadersInterface = {
listing_info: 'Let’s get started! This section will inform guests about where they’ll be staying and what to expect. The more descriptive, the better.',
accommodations: 'accommodations header',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we need the word header at the end of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeholders again -- I'll swap in the actual headers

@woeltjen
Copy link
Contributor

woeltjen commented Feb 1, 2019

This is looking good! 👍 to using position: sticky for the aside

@jerminatorhits jerminatorhits changed the title listing-form-sidebar-help (WIP visit https://dev03.beenest.io/host/listings/87134/listing_info) listing-form-sidebar-help (visit https://dev03.beenest.io/host/listings/87134/listing_info) Feb 5, 2019
@jerminatorhits
Copy link
Contributor Author

jerminatorhits commented Feb 5, 2019

Most field helpers have been filled out. Still improving the content with Amy on Tuesday but for the most part, the structure is there. Feel free to comment on the smallest details / typos; be as nitpicky as possible (within reason) thx! 😄

<InputWrapper>
<Field
name="sleepingArrangement"
onFocus={() => setFocus('sleepingArrangement')}
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a little easier to manage these as enums so you can just change it in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use LISTING_FIELDS enum

<h4>Examples:</h4>
<br />
<div className="images-container">
<LazyImage className="image-container--horizontal" src="https://s3-us-west-2.amazonaws.com/beenest-public/images/photo-examples/Do_LA_Bedroom.jpg" />
Copy link
Contributor

Choose a reason for hiding this comment

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

small comment but can we use the https://static.beenest.com url for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@kcvan
Copy link
Contributor

kcvan commented Feb 6, 2019

just a few things that might not be in the scope of this pr:

  1. the plus and minus buttons are tabbable but the outline isnt showing
  2. the checkbox doesnt seem tabbable

somethings we can change in this pr:
Min Nights might want to be changed to Minimum Nights for the label and the description

@kcvan
Copy link
Contributor

kcvan commented Feb 6, 2019

also, is there a reason why the help description is not highlightable?

@jerminatorhits
Copy link
Contributor Author

jerminatorhits commented Feb 6, 2019

reason help text isn't highlightable is z-index: -1. This is due to the position: fixed issue + the verification banner that pushes everything down except fixed position divs. Could fix by removing -1 z index and not letting unverified users create listings so the banner never shows up. motion to fix in new pr.

@jerminatorhits
Copy link
Contributor Author

good calls. Will update min nights to more complete Minimum Nights also down to look into the tabbable issues in another pr

</ListItem>
<ListItem noHover prefixColor="correct" start="tiny">
<Svg className="prefix" src="utils/check-circle" />
<span>Do provide multiple angles</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is preceded with a "Do:" (the h4 above) I'd just say "Provide multiple angles" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx updated.

path="/host/listings/:id/accommodations"
render={() => <AccommodationsForm {...FormikProps} />}
render={() => (
<AccommodationsForm {...FormikProps} ListingField={ListingField} setFocus={this.handleFocus} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing in the ListingField enum as a prop here feels funny; could ListingField just be exported from somewhere appropriate and shared by the various form pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was thinking the same. Any suggestions on where it belongs? I was thinking networking/listings but it might be out of scope for that file

Copy link
Contributor

Choose a reason for hiding this comment

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

networking/listings was my first thought as well. Agree that it isn't ideal w.r.t. the scope of that file; maybe leave it there with a TODO prompting us to move it when we have a more appropriate place for form descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm. this should make the form much cleaner!


checkInTime: <CheckInTime />,
checkOutTime: <CheckOutTime />,
houseRules: <HouseRules />,
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this approach!

Would [ListingField.HOUSE_RULES]: <HouseRules /> make sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Oh ya definitely could use that here as well

import LazyImage from 'shared/LazyImage';

interface ListingHelpTextInterfaceInterface {
[name: string]: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

ListingHelpTextInterfaceInterface gives me many giggles. Can we name this ListingHelpTextInterface and change the returned const to ListingHelpText? That gives me no giggles at all.

Or even just ListingHelp

Oh, and corollary to comment below, think this could be:

[name: ListingField]: React.ReactNode;

...to leverage type-checking a little more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops InterfaceInterface was from me being vscode trigger happy. good catch!

Copy link
Contributor Author

@jerminatorhits jerminatorhits Feb 6, 2019

Choose a reason for hiding this comment

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

condensed filename to ListingHelp

tried to use the enum in building the interface but typescript doesn't like it
image

Looked at few sources and it might require some golfing.
microsoft/TypeScript#5683
https://stackoverflow.com/questions/50849359/how-can-i-use-a-string-enum-type-as-a-computed-property-name-in-an-interface

@woeltjen
Copy link
Contributor

woeltjen commented Feb 6, 2019

A few minor suggestions inline. This looks great and I liked testing it!

Copy link
Contributor

@tc tc left a comment

Choose a reason for hiding this comment

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

looking good!

@jerminatorhits jerminatorhits merged commit 70c55a2 into master Feb 7, 2019
@jerminatorhits jerminatorhits deleted the jeremy/ENG-920-listing-helper-section branch February 7, 2019 23:16
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.

4 participants