-
Notifications
You must be signed in to change notification settings - Fork 4
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
Enable address editing #1564
Enable address editing #1564
Conversation
@@ -160,8 +173,10 @@ const DataTable = ({ | |||
autoFocus | |||
name={field} | |||
id={field} | |||
onChange={e => handleInputChange(e)} | |||
defaultValue={fieldValue} |
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.
im updating all of these inputs to be controlled components instead of uncontrolled
editor/src/Components/DataTable.js
Outdated
onChange={e => handleInputChange(e)} | ||
value={editValue | ||
.toString() | ||
.toUpperCase()} |
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.
updating these to be controlled components allows me to auto capitalize user input as they type
const [isConfirmModalOpen, setIsConfirmModalOpen] = useState(false); | ||
|
||
const crash = data?.crash; | ||
|
||
const toggleModal = () => { | ||
// make sure we are not in edit mode on a field | ||
setEditField(""); |
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.
so that if we are in edit mode on a field and click the swap address button, we get out of edit mode
value={editValue} | ||
onChange={e => | ||
setEditValue( | ||
e.target.value.toUpperCase() |
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.
auto capitalize user input
@@ -1,12 +1,12 @@ | |||
{ | |||
"name": "atd-vz-data", | |||
"version": "1.47.0", | |||
"version": "2.1.0", |
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.
from running npm install
for the first time in awhile
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 tried editing fields, quitting editing halfway through, opening them again, swapping addresses mid edit, and it all works as you described it should.
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.
yay, address editing! 🙌
and thanks for switching these over to controlled components. i'm seeing a few warnings that should be a quick fix.
also, one result of this change is that the previous value flashes on the screen while the save + refetch happens. can you see if there's a fix for this maybe in the handleFieldUpdate
callback?
🙏
editor/src/Components/DataTable.js
Outdated
@@ -33,13 +34,22 @@ const DataTable = ({ | |||
const roles = getRoles(); | |||
const isReadOnlyUser = isReadOnly(roles); | |||
|
|||
// Sets the state of the value for the current field being edited | |||
const [editValue, setEditValue] = useState(); |
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.
eslint is barking because null
/undefined
is not a valid value to pass to input
elements. we need an empty string as the default value and whenever you reset the value.
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'm also getting this warning when i start editing a blank field:
Warning: A component is changing an uncontrolled input of type text to be controlled.
i think this might resolve itself if you make default input type a string.
editor/src/Components/DataTable.js
Outdated
// Import Lookup tables and aggregate an object of uiType= "select" options | ||
const { data: lookupSelectOptions } = useQuery(GET_LOOKUPS); | ||
|
||
const handleEditClick = (field, fieldValue) => { | ||
setEditField(field); | ||
setEditValue(fieldValue); |
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.
here to you need to set the edit value to ""
if it is null
@johnclary this is ready for re-review! |
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.
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 replicated the bug that John caught too, and, after that, this is good to go! I was able to edit every address field and all other select (non-boolean) and text type fields too.
@johnclary @mddilley thanks for catching that guys! it had to do with the way i was checking for falsey values in general instead of explicitly checking if the edit value was null to set it to an empty string in the |
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.
🚢 🚢 🚢 🚢
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.
Awesome! I tested again with the updated handling and it is ✨
Associated issues
Closes cityofaustin/atd-data-tech#19014
Testing
URL to test:
Netlify
Steps to test:
Ship list
main
branch