Skip to content

Conversation

@aggarwalpulkit596
Copy link
Contributor

@aggarwalpulkit596 aggarwalpulkit596 commented Apr 1, 2019

Fixes #1421

Changes: Added Popular Location in search location fragment

Screenshots for the change:
screenshot-1554308149173

@angmas1
Copy link
Contributor

angmas1 commented Apr 1, 2019

Add screenshot

@aggarwalpulkit596
Copy link
Contributor Author

@angmas1 i haven't completed the pr as of yet that's why i added a WIP tag

@angmas1
Copy link
Contributor

angmas1 commented Apr 1, 2019

@angmas1 i haven't completed the pr as of yet that's why i added a WIP tag

Okay, sorry continue working.

@aggarwalpulkit596 aggarwalpulkit596 changed the title [WIP]feat: add Event Location[skip ci] feat: add Event Location Apr 1, 2019
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal a little bit of help here as autocompletefragment requires match parent layout and i'm not able to add it with a list view any suggestions

@iamareebjamal
Copy link
Member

I don't understand

@fossasia fossasia deleted a comment Apr 2, 2019
@fossasia fossasia deleted a comment Apr 2, 2019
@iamareebjamal iamareebjamal added this to the v0.2.0 milestone Apr 2, 2019
@iamareebjamal
Copy link
Member

This is not correct UI. You'll have to change it's behavior

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal this can be merged after mapbox have been removed

@angmas1
Copy link
Contributor

angmas1 commented Apr 3, 2019

UI looks bad, I think we should stop showing popular cities list once user starts typing.

@aggarwalpulkit596
Copy link
Contributor Author

Thing's would be different after removing the mapbox sdk autocomplete textview would be removed and also in terms of UI it is pretty much similar to Eventbrite

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the popularLocations branch 2 times, most recently from d5aa41b to a0ec2d1 Compare April 3, 2019 16:14
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal i've refractor constraint layout to linear layout as there is no use of constraint layout for this layout and we should avoid using constraint layout as it much more expensive than linear layout.

@iamareebjamal
Copy link
Member

iamareebjamal commented Apr 3, 2019

Peer reviews first

@aggarwalpulkit596
Copy link
Contributor Author

@liveHarshit please review

@aggarwalpulkit596
Copy link
Contributor Author

@liveHarshit updated

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the popularLocations branch 2 times, most recently from 8a373c6 to be4a014 Compare April 4, 2019 16:01
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal i messed up the pr a little bit should i make seperate pr because i'm not able to fix it

@fossasia fossasia deleted a comment Apr 6, 2019
@iamareebjamal
Copy link
Member

You can create a new branch and force push it on this branch

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal it has been fixed please review

else
eventLocationApi.getEventLocation()
.map {
eventLocationDao.insertEventLocations(it)
Copy link
Member

Choose a reason for hiding this comment

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

No need to save in DB

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated

iamareebjamal
iamareebjamal previously approved these changes Apr 7, 2019
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal fixed conflicts

iamareebjamal
iamareebjamal previously approved these changes Apr 7, 2019
@aggarwalpulkit596
Copy link
Contributor Author

I'm getting an error that dryRun has bee deprecated

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal please review

@Id(LongIdHandler::class)
val id: Long?,
val name: String?,
val slug: String?
Copy link
Member

Choose a reason for hiding this comment

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

Why are these nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can intialize them with empty string also i just followed the pattern which was found in most of the data classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamareebjamal should i initialize with them empty string instead of being nullable type?

Copy link
Member

Choose a reason for hiding this comment

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

No, make them non nullable without intializers. Tell me if they don't work

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated and it is working even without being of type and also in JSON API responses the id are usually not nullable so should i refractor all them in a separate PR

iamareebjamal
iamareebjamal previously approved these changes Apr 9, 2019
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated

@iamareebjamal iamareebjamal merged commit 75180e7 into fossasia:development Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants