Extract booking section from listing page #969
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lyyder
force-pushed
the
extract-section-booking
branch
3 times, most recently
from
3ea3d1f
to
d40582b
Compare
lyyder
force-pushed
the
extract-section-booking
branch
2 times, most recently
from
d84f14e
to
b36a367
Compare
Gnito
reviewed
Dec 5, 2018
| @@ -110,6 +110,10 @@ | |||
| font-weight: 400; | |||
| } | |||
|
|
|||
| & :global(.DayPicker__withBorder) { | |||
There was a problem hiding this comment.
What does this do? Is there possibility that mobile layout or desktop layout breaks?
There was a problem hiding this comment.
@Gnito removes rounding from the corners of the calendar. I guess we might be able to live without this as the border-radius is somehow not applied when there's 312px available for the calendar. Just added this as I wanted to explicitly remove the rounding on the corners and not by accident.
lyyder
force-pushed
the
extract-section-booking
branch
from
d7de7fb
to
34183f0
Compare
lyyder
force-pushed
the
extract-section-booking
branch
2 times, most recently
from
9ea2c54
to
9b133cd
Compare
lyyder
force-pushed
the
extract-section-booking
branch
2 times, most recently
from
ec2998d
to
2b60890
Compare
Gnito
reviewed
Dec 19, 2018
src/translations/en.json
Outdated
| "BookingPanel.closedListingButtonText": "Sorry, this listing has been closed.", | ||
| "BookingPanel.ctaButtonMessage": "Request to book", | ||
| "BookingPanel.hostedBy": "Hosted by {name}", | ||
| "BookingPanel.perUnit": "per night", |
Gnito
reviewed
Dec 19, 2018
| @@ -264,6 +229,11 @@ export class ListingPageComponent extends Component { | |||
| </span> | |||
| ); | |||
|
|
|||
| const bookingTitle = ( | |||
| <FormattedMessage id="ListingPage.bookingTitle" values={{ title: richTitle }} /> | |||
There was a problem hiding this comment.
It might make sense to
- remove unnecessary span (
<FormattedMessage>) and - use
intl.formatMessage({ id: 'ListingPage.bookingSubTitle' }, { title: richTitle });instead.
There was a problem hiding this comment.
@Gnito It appears that this requires the FormattedMessage component. Or at least using the intl.formatMessage like that renders the title param as [Object object] if a node is passed there.
Gnito
reviewed
Dec 19, 2018
| isOwnListing: bool, | ||
| unitType: propTypes.bookingUnitType, | ||
| handleBookingSubmit: func.isRequired, | ||
| title: oneOfType([object, string]).isRequired, |
There was a problem hiding this comment.
node is probably better.
https://www.npmjs.com/package/prop-types
// Anything that can be rendered: numbers, strings, elements or an array
// (or fragment) containing these types.
Gnito
reviewed
Dec 19, 2018
| import { FormattedMessage } from 'react-intl'; | ||
| import classNames from 'classnames'; | ||
| import omit from 'lodash/omit'; | ||
| import { ModalInMobile, Button } from '../../components'; |
There was a problem hiding this comment.
Import components (incl. forms) as a last thing in imports from outside of this directory.
aka
// import dependency libs
import React from 'react';
// import utils
import { formatMoney } from '../../util/currency';
// import components
import { ModalInMobile, Button } from '../../components';
// import stuff from this directory
import css from './BookingPanel.css';
- This makes these imports easier to read and
- It also makes it less likely to get strange dependency graphs since components might require same utilities than this one.
(I've tried to enforce this structure with everyone.)
Gnito
requested changes
Dec 19, 2018
|
Since this touches modals: test heavily with mobile phones. |
lyyder
force-pushed
the
extract-section-booking
branch
from
f7407da
to
299750d
Compare
Gnito
approved these changes
Dec 20, 2018
Extract SectionBooking away from ListingPage to a distinct component called BookingPanel.
Remove isClosed and price from the BookingPanel props as they can be resolved from the listing.
Rename the prop to title and allow it to be and object or a string. This way a normal string or a styled html element can be provided.
Use title and sub title provided by a parent component.
Change formatted title and sub title type to node in BookingPanel. Also remove FormattedText component which renders a redundant span element.
Place components and forms as last imports before the ones from the same folder.
lyyder
force-pushed
the
extract-section-booking
branch
from
c4d9e47
to
3d11d73
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extract listing page's internal component
SectionBookinginto it's own component calledBookingPanel.The idea is to handle opening and closing of the mobile modal inside the component and only take submit handler and
onManageDisableSrcollingas props.