Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:added location field #270

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

aakankshadhurandhar
Copy link

Fixes : #257
Screenshot from 2022-09-11 20-04-30

Comment on lines -3 to +4
<Input
<div class="field">
<Input
Copy link
Contributor

@vinayak-trivedi vinayak-trivedi Sep 13, 2022

Choose a reason for hiding this comment

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

Do we need this div field for each input, can we put a separate if statement which check if the field is location, and then renders a different form input which is inside a div and contains a button

Copy link
Author

Choose a reason for hiding this comment

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

@vinayak-trivedi If we are doing this it is breaking whole form css of the form

@vinayak-trivedi
Copy link
Contributor

one more thing can we increase the margin left and right for the button

@rajat-mehra05
Copy link

one more thing can we increase the margin left and right for the button

You mean padding right ? Playing with margin property breaks css frequently for other elements as well.

@vinayak-trivedi
Copy link
Contributor

one more thing can we increase the margin left and right for the button

You mean padding right ? Playing with margin property breaks css frequently for other elements as well.

yes padding. thanks for pointing it out rajat

@vinayak-trivedi
Copy link
Contributor

one more thing @aakankshadhurandhar you should also send this data with other data to backend also

@vinayak-trivedi
Copy link
Contributor

vinayak-trivedi commented Sep 18, 2022

@aakankshadhurandhar we also need to give the user the suggestion of the location as shown in the ss below, and we will store the coordinates of that location, we will be using geocoding API for that, please have a look at that

image

@aakankshadhurandhar
Copy link
Author

@vinayak-trivedi I think do you mean integrating of geolocation API with the update location button? I think we must do this in a separate PR.

@vinayak-trivedi
Copy link
Contributor

@vinayak-trivedi I think do you mean integrating of geolocation API with the update location button? I think we must do this in a separate PR.

yes makes sense, ensure one thing that even if it goes to prod, the user do not get to fill this location field for now, it should only be visible if dev=true

@vinayak-trivedi
Copy link
Contributor

@vinayak-trivedi I think do you mean integrating of geolocation API with the update location button? I think we must do this in a separate PR.

yes makes sense, ensure one thing that even if it goes to prod, the user do not get to fill this location field for now, it should only be visible if dev=true

I don't think that this is being addressed here, we need to show this field only if we havedev=true in query params

@vinayak-trivedi
Copy link
Contributor

One more change that is required is that the user should only see the update location button if he has already filled the location

@@ -9,6 +10,13 @@
@class={{if @showError 'input errorBorder' 'input'}}
{{on 'input' (fn this.inputFieldChanged)}}
/>
{{#if @buttonupload }}
<button class="upload" type="submit">update location</button>
Copy link
Contributor

@rohan09-raj rohan09-raj Nov 23, 2022

Choose a reason for hiding this comment

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

update location -> Update, as the user already knows they are updating the location from the Input Label

Comment on lines +18 to +19


Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please remove these extra spaces?

label: 'Location',
type: 'text',
placeholder: 'bangalore',
buttonupload: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please follow CamelCase here?

@@ -28,6 +28,10 @@ h1 {
flex-direction: column;
justify-content: center;
align-items: flex-start;
position: relative;
}
.field_right{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please put a single line space after the CSS block?

.container {
width: 100%;
color: #666;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please use CSS variables here?
Please follow the pattern in variables.css file

.user-details__heading {
font-size: 2rem;
color: black;
Copy link
Contributor

Choose a reason for hiding this comment

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

width: 100%;
border-radius: 10px;
border: 2px solid darkgray;
Copy link
Contributor

Choose a reason for hiding this comment

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

border-radius: 10px;
outline: none;
background-color: green;
Copy link
Contributor

Choose a reason for hiding this comment

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

outline: none;
background-color: green;
color: white;
Copy link
Contributor

Choose a reason for hiding this comment

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


.signup-action__btn button.btn-disabled {
background: #ccc;
Copy link
Contributor

Choose a reason for hiding this comment

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

right: 0;
height: 100%;
color: blue;
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -16,7 +16,10 @@
@helpMsg={{field.helpMsg}}
@onChange={{this.handleFieldChange}}
@validator={{field.validator}}
@disabled={{field.disabled}} />
@disabled={{field.disabled}}
@buttonupload={{field.buttonupload}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please follow CamelCase here?

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.

Add location field
4 participants