-
Notifications
You must be signed in to change notification settings - Fork 2
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
task/WG-74: Create Map React Modal #193
Conversation
react/package-lock.json
Outdated
@@ -1,1759 +1,1031 @@ | |||
{ | |||
"name": "hazmapper", | |||
"version": "0.0.0", | |||
"lockfileVersion": 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you recreate your package-lock.json. you should switch to a newer version of npm node (via nvm). like npm version 20.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. 👍 I left a variety of comments and suggestions. We can address some in this PR and then create follow on tickets for other ones (i.e. things that take longer)
react/package.json
Outdated
"@testing-library/react": "^13.4.0", | ||
"@types/leaflet.markercluster": "^1.5.1", | ||
"axios": "^1.6.2", | ||
"bootstrap": "^5.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can bootstrap package be removed now that we are using the cdn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should still work with just the cdn. Let me test that though
const mockUsername = 'mockUser'; | ||
const mockEmail = '[email protected]'; | ||
|
||
const mockToken = { | ||
token: 'mockTokenValue', | ||
expires: Date.now() + 1000 * 60 * 60, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the entries (like authenticatedUser
in react/src/__fixtures__/authStateFixtures.ts
could be used here (and they can be altered or extended if they don't cover what is needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part can be deleted once useName (WG-219) becomes available. I wonder if I should just pull that branch into this one, so I can use it for the unit test and MapModal comp?
@@ -0,0 +1,200 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: what about making the name more specific? something likeMapCreateModal
instead of MapModal
? 🤷♂️
or NewMapModal? or CreateMapModal
<FormGroup className="row align-items-center"> | ||
<Label className="col-sm-4">Save Location:</Label> | ||
<div className="col-sm-8 text-primary"> | ||
{user ? `/${user.name}` : 'Loading...'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a comment (with TODO_REACT) in it here, reminding us. or maybe this one is too obvious for a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can add a comment reminding us that this part will change once the File Browser comp is built
<FormGroup> | ||
<Label for="system_file">Custom File Name</Label> | ||
<div className="input-group"> | ||
<Field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we match the behavior on the current app where the user inputs the map name and the file initially matches the map name? Lets add a follow on Jira issue
|
||
function MainMenu() { | ||
const { data, isLoading, error } = useProjects(); | ||
const { mutate: createProject, isLoading: isCreatingProject } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could line 14 and the handleCreateProject (20-33) be moved into the MapModal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
toggleModal(); | ||
}, | ||
onError: (err) => { | ||
// Handle error (e.g., show error message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to not let the modal close and then show an error in the modal for users to see.
we should match the current behavior where we show the error message or the text " That folder is already syncing with a different map!"
const handleCreateProject = (projectData) => { | ||
createProject(projectData, { | ||
onSuccess: () => { | ||
// TO-ADD: Handle success (e.g., show success message, refetch projects, etc.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to navigate to the new map as now we know the new uuid:
import { useNavigate } from 'react-router-dom';
import { Project } from '../../types';
....
onSuccess: (newProject: Project) => {
navigate(`/project/${newProject.uuid}`);
},
this /project/uuid route is already supported in react/src/AppRouter.tsx
and bring us to a MapProject page (with fixtures for map data)
size="short" | ||
type="primary" | ||
attr="submit" | ||
disabled={isCreating} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use isLoading
instead of disabled
property here. the isLoading disables the button and gives us a loading spinner as we wait for the action to occur. Then the shown text would just be "Create"
}; | ||
return ( | ||
<Modal isOpen={isOpen} toggle={toggle}> | ||
<ModalHeader toggle={toggle}>Create a New Map</ModalHeader> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ModalHeader toggle={toggle}>Create a New Map</ModalHeader> | |
<ModalHeader toggle={toggle} charCode="">Create a New Map</ModalHeader> |
Could we use this and then drop the related button close CSS from CreateMapModal.module.css?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work for me. Don't think ModalHeader has charCode as a prop? I'd have to add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. we did it that way in CEP. wonder what the difference is. maybe dropped from bootstrap or react-strap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look in CEP and see if I can find an example. Normally the close button is built in with the react-strap ModalHeader, but bc we're using a lower version of bootstrap in our project, that btn-close class doesn't exist. But if I can find it in CEP, I'll use that code here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it is used like that in CEP:
<ModalHeader toggle={toggle} charCode="">
Authenticate with TACC Token
</ModalHeader>
I'll try to figure out why it didn't work for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* To render modal close button icon as a Cortal icon /
/ CAVEAT: Pass charCode=""
to <ModalHeader>
/
.modal-header .close span {
/ To mimic .icon
styles without @extend
or composes
(unavailable) /
/ HACK: Copied (and reduced and edited) from src/styles/trumps/icon...
/
font-size: 1.5rem; / bigger to match header text font height (like design) */
font-family: Cortal-Icons !important;
}
Looks like a custom handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. let's ignore and revisit whenever we do another modal.
Looks like a custom handling
thanks for tracking that down.
validationSchema={validationSchema} | ||
onSubmit={handleSubmit} | ||
> | ||
{/*TODO-REACT: Will change to core-style's FormField comp instead of Formik's Form & Field comp*/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the Jira issue here and/or in PR decription?
and TODO-REACT -> TODO_REACT
useCreateProject(); | ||
const navigate = useNavigate(); | ||
|
||
const handleCreateProject = (projectData, actions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const handleCreateProject = (projectData, actions) => { | |
const handleCreateProject = (projectData: ProjectRequest, actions) => { |
would be good to use type here.
</div> | ||
)} | ||
</FormGroup> | ||
{/* TODO-REACT: This part will change once the FileBrowser component is added*/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{/* TODO-REACT: This part will change once the FileBrowser component is added*/} | |
{/* TODO_REACT: This part will change once the FileBrowser component is added. https://tacc-main.atlassian.net/browse/WG-208*/} |
}); | ||
}; | ||
|
||
const handleSubmit = (values, actions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the actions
and actions.setSubmitting
needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not necessary, but keeping track of the submitting state might be helpful down the road when/if we add the feature to disable the Submit button, while the form is still handling the POST request? But I can take it out and we can add it back at that time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but I think you have that case covered already by using isCreatingProject
and the Button
's isLoading parameter which adds a spinner and disables the button:
<Button
size="short"
type="primary"
attr="submit"
isLoading={isCreatingProject}
>
Create
</Button>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yep you're right. Forgot we're using the core-style's Button wrapper now. I'll take the code out.
react/src/types/projects.ts
Outdated
observable?: boolean; | ||
watch_content?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about making these not optional
observable?: boolean; | |
watch_content?: boolean; | |
observable: boolean; | |
watch_content: boolean; |
setErrorMessage('An error occurred while creating the project.'); | ||
if (err?.response?.status === 409) { | ||
setErrorMessage( | ||
'That folder is already syncing with a different map.' | ||
); | ||
} else if (err?.response?.status === 500) { | ||
setErrorMessage('Internal server error. Please contact support.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about just having two types of error messages? one for the 409 and one for just the general error?
setErrorMessage('An error occurred while creating the project.'); | |
if (err?.response?.status === 409) { | |
setErrorMessage( | |
'That folder is already syncing with a different map.' | |
); | |
} else if (err?.response?.status === 500) { | |
setErrorMessage('Internal server error. Please contact support.'); | |
} | |
if (err?.response?.status === 409) { | |
setErrorMessage( | |
'That folder is already syncing with a different map.' | |
); | |
} else { | |
setErrorMessage('An error occurred while creating the project. Please contact support.'); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Overview:
Create a new map modal in react. See the current angular "Create new map" modal for comparison.
skip doing the file/project browser as it requires to much work.
Add POST request to Geoapi's /projectsendpoint by adding/using a new hook in react/src/hooks (and due to being blocked by WG-208, it defaults to always creating a map linked with the users My Data.)
PR Status:
Related Jira tickets:
Summary of Changes:
Testing Steps:
UI Photos:
Core Styles Added:
Notes:
This new Modal will require follow-up work relating to WG-246 (using FieldWrapperFormik), as well as adding the feature/behavior where the filename is updated by the map name (unless the user manually changes the filename after). See https://tacc-main.atlassian.net/browse/WG-249.