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

simplify FileBrowser selection code, fix bracket lints #463

Merged

Conversation

kostmo
Copy link
Contributor

@kostmo kostmo commented May 6, 2023

No description provided.

@jtdaugherty
Copy link
Owner

Hi @kostmo, thanks for your patch. Could you share a bit about what motivated these changes? Was this the result of running a linter, or did you want to make these changes as a result of looking at the code to understand something?

@kostmo
Copy link
Contributor Author

kostmo commented May 6, 2023

what motivated these changes

Hi, a little of both---for swarm-game/swarm#1010 I want to configure the FileBrowser dialog to close upon selection of just a single file, rather than marking multiple files. So I found myself studying the maybeSelectCurrentEntry function, which was a bit cumbersome to read. Meanwhile the linter was yelling to fix parentheses in the same file :). Ultimately I found a technique in the FileBrowser demo.

@jtdaugherty
Copy link
Owner

Okay, thanks for the context!

I'll merge this, but I have a request for future patches, if you submit any: please make atomic commits that make one logical (type of) change per commit. Concretely, I'd have liked the the single commit in this PR to be two commits: one to change only the parens, and one to refactor existing code. Commits that make multiple changes that are unrelated to each other can be more difficult to review and are not as amenable to git bisect in case of a future problem. If you don't know, git add -p is the best way to do this if you find yourself making unrelated changes to the same file; that will prompt per-hunk so you can stage just the right changes and leave others behind for subsequent commit(s).

@jtdaugherty jtdaugherty merged commit 1cc1845 into jtdaugherty:master May 6, 2023
8 checks passed
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.

None yet

2 participants