Map Circle Warning #1456
Merged
Map Circle Warning #1456
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
…ge to make sure the circle is not centered over the address..
… this one due to how many editable options there are.
|
@AllhandsNoFeet thoughts? |
lucaslcode
requested changes
Jun 22, 2021
There was a problem hiding this comment.
Seems good to me for now.
Could you change ConfirmationDialogWrapper.tsx so it has a prop that lets you choose between "Okay" and "Yes" as the 'accept button'. Like confirmButtonLabel
I also noticed that if there's a server error ("you must be 18 yo to sign up"), it is hard to notice because the submit button is at the bottom of a long form.
Could you add this to signup.tsx
useEffect(() => {
if (authState.error) window.scroll({ top: 0, behavior: "smooth" });
}, [authState.error]);
lucaslcode
reviewed
Jun 22, 2021
| @@ -363,6 +364,9 @@ function RadiusSlider({ commit, initialRadius, redrawMap }: RadiusSliderProps) { | |||
| const [radius, setRadius] = useState(initialRadius); | |||
| return ( | |||
| <> | |||
| <Typography color="secondary" gutterBottom={true}> | |||
There was a problem hiding this comment.
Suggested change
| <Typography color="secondary" gutterBottom={true}> | |
| <Typography variant="body2" gutterBottom> |
I think that's sufficient
|
Cool on it! |
…to signup, made the adjustment on EditLocationMap
lucaslcode
requested changes
Jun 24, 2021
| @@ -37,7 +39,9 @@ export default function ConfirmationDialogWrapper({ | |||
| <DialogContentText>{message}</DialogContentText> | |||
| </DialogContent> | |||
| <DialogActions> | |||
| <Button onClick={handleConfirm}>Yes</Button> | |||
| <Button onClick={handleConfirm}> | |||
| {confirmButtonLabel ? confirmButtonLabel : "Yes"} | |||
There was a problem hiding this comment.
Better move the default to a constant (sorry)
|
Is this ready to merge? If so, feel free to merge it yourself @JGibbsWork |
|
Sorry about that! All set now! |
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.
Updated signup to include a confirmation dialogue wrapper warning about the circle surrounding the address. Not sure if we want to create a barrier for signing up, but it did seem like a more direct way to solve the problem for new users. Also added text below the map when editing your profile. More than happy to change either accordingly.
Frontend checklist
yarn format && yarn lint --fixyarn lint