-
Notifications
You must be signed in to change notification settings - Fork 80
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
Handle pasted data types #989
Conversation
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.
Code looks really good, thanks for the update! One thing we could do if you're up for it is to validate the user's input and reject it rather than setting the time to 00:00:00 on invalid input in the time columns.
I need to do $FlowFixMe instead |
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.
Couple of small comments and the behaviour saving the current contents when invalid time formats are entered doesn't seem to be working 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.
Works well for me! Thanks for the update
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.
Great work! Lots of good cleanup! Just a little bit more
beginEditing () { | ||
const {columnIndex, rowIndex, setActiveCell} = this.props | ||
const beginEditing = () => { | ||
const {columnIndex, rowIndex, setActiveCell} = props |
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.
Why are these variables defined?
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 console was mad when I didn't define the variables since I switched everything to a functonal component
lib/editor/util/timetable.js
Outdated
clearTimeout(alertTimeout) | ||
if (isTimeFormat(col.type)) { | ||
if (!parsedDate.isValid()) { | ||
alertTimeout = setTimeout(() => alert('Please enter a valid time format'), 600) |
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.
Why do we wait to alert the user?
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 prevented an issue where the alert would show twice, I can make the timeout shorter
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.
See my changes for a fix following this PR's approach to solving this problem, but it sounds like @philip-cline might have an alternate solution cooking
Checklist
dev
before they can be merged tomaster
)Description
parseCellValue
can parse the correct string inTIMETABLE_FORMATS