Closed maxbergmark closed 1 week ago
Thanks @maxbergmark! I'll make sure to review this over the weekend.
@friendlymatthew All good suggestions, I fixed them immediately. I also noticed that #127 causes conflicts with my changes. I changed my code to also use key()
instead of code()
, and handled those conflicts.
@friendlymatthew Good suggestion, I'll clean it up.
A quick note: I got a bit worried that I'd broken something: Due to #127, there might need to be some updates to the documentation. I noticed that all my hotkeys that were set up using alphanumeric keys weren't working. After some digging, it seems that the reason is that the event code and the event key are different. While the event code is e.g. "KeyA"
or "Digit1"
, the event key is just "A"
or "1"
. After updating all keyboard shortcuts in my repository it works just like before, but I would describe this as a breaking change unfortunately.
~I hope I'm mistaken here and there's something I'm overlooking, but it is something that should be noted and tested thoroughly.~ I looked at the original issue #121 which included links to the documentation of code()
and key()
. It seems that the breaking change indeed comes from there.
@all-contributors please add @maxbergmark for code
@friendlymatthew
I've put up a pull request to add @maxbergmark! :tada:
Hey @maxbergmark, what do you say we rollback on these two commits, and make these their own PRs? That way we can get this merged in at the time 1afdfb5c1e91b641ebe6aa423235eaea338d5c89
@friendlymatthew Sounds like a good plan, it's better practice to have separate PRs.
@friendlymatthew Sounds like a good plan, it's better practice to have separate PRs.
Great, let me know when you do that. I'll take a final look and then should be good to merge.
@friendlymatthew I reverted the past two commits
LET'S GOOOOOOOOO! So awesome to see this over the line @maxbergmark!
Just a note, we'll need to immediately handle the
.key()
calling convention. #135
Also, thank you for spending time on this!
Thank you for approving this! If you merge it (I don't have write access) you could re-use the logic for using either the key or code:
#[cfg(not(feature = "ssr"))]
fn clean_key(event: &web_sys::KeyboardEvent) -> String {
if cfg!(feature = "FEATURE_NAME") {
match event.key().as_str() {
" " => "spacebar".to_string(),
key => key.to_lowercase(),
}
} else {
event.code()
}
}
Thank you for approving this! If you merge it (I don't have write access) you could re-use the logic for using either the key or code:
#[cfg(not(feature = "ssr"))] fn clean_key(event: &web_sys::KeyboardEvent) -> String { if cfg!(feature = "FEATURE_NAME") { match event.key().as_str() { " " => "spacebar".to_string(), key => key.to_lowercase(), } } else { event.code() } }
Hmm, I sent an invitation to have write access to this crate for both you and @rkimoakbioinformatics.
Seems like your invite got expired, I just resent it, can you check your email/Github notifications?
Just a note: fixes #119