-
Notifications
You must be signed in to change notification settings - Fork 29
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
menu/UI cleanup #3: click-to-edit on some columns #851
Conversation
23207c6
to
773ec99
Compare
773ec99
to
5444e31
Compare
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 are we confident in merging this in at this point? Think it would be good to be able to test and feel confident about this kind of change so close to hunt. @rawxfish what do you think?
Couple comments inline
import type { Hunt, Row } from "./types"; | ||
|
||
export default function NotesCell({ row, value }: { row: Row; value: string }) { | ||
const { id: huntId } = useSelector<RootState, Hunt>((state) => state.hunt); | ||
const [uiHovered, setUiHovered] = React.useState(false); | ||
const dispatch = useDispatch(); | ||
const [uiHovered, setUiHovered] = useState(false); |
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.
For CSS stuff I think it is much preferable to use :hover
to control the interactions rather than to manage it in JS when possible. The JS way is kind of fragile and error prone.
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.
sorry i jumped the gun on merging this, fixed in #854
setEditedNotesValue(e.target.value); | ||
}} | ||
onKeyDown={(e) => { | ||
if (e.key === "Enter" && (e.ctrlKey || e.shiftKey || e.metaKey)) { |
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.
Isn't this a reverse of normal semantics? Normally enter is submit and shift+enter is new line right?
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.
yeah i thought about this, but notes are often multiline, and i think people are going to get more confused by pressing enter and having it submit. modifier + enter
isn't very discoverable, but there's a checkmark button at least
this removes the icons entirely. instead, clickable cells will now highlight (with a yellow background) when hovered, indicating an action can be taken. (sorry idk how to take a video) (also sorry the bg is kinda ugly but it's harder to fix because of the
react-table
component we use)resolves #835