Skip to content
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

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

npinsker
Copy link
Collaborator

@npinsker npinsker commented Jan 15, 2025

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)

image
  • clicking on Name column opens up puzzle edit dialog
  • clicking on Tags column opens up tag edit dialog
  • clicking on the Notes column just turns the cell into an inline editable textbox instead of opening up a modal
image

resolves #835

@npinsker npinsker force-pushed the npinsker/refactor-menus-3 branch from 773ec99 to 5444e31 Compare January 16, 2025 00:52
@npinsker npinsker marked this pull request as ready for review January 16, 2025 00:53
@npinsker npinsker changed the title click-to-edit on some columns menu cleanup #3: click-to-edit on some columns Jan 16, 2025
@npinsker npinsker changed the title menu cleanup #3: click-to-edit on some columns menu/UI cleanup #3: click-to-edit on some columns Jan 16, 2025
hunts/src/NotesCell.tsx Outdated Show resolved Hide resolved
hunts/src/NotesCell.tsx Show resolved Hide resolved
hunts/src/TagCell.tsx Show resolved Hide resolved
hunts/src/NotesCell.tsx Show resolved Hide resolved
@npinsker npinsker merged commit 02eead3 into main Jan 16, 2025
2 checks passed
@npinsker npinsker deleted the npinsker/refactor-menus-3 branch January 16, 2025 06:16
Copy link
Contributor

@rgossiaux rgossiaux left a 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);
Copy link
Contributor

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.

Copy link
Collaborator Author

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)) {
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify three different buttons & menus for 'edit puzzle', 'tag puzzle', and 'delete puzzle' into one
3 participants