Skip to content

Conversation

@oxcrag
Copy link

@oxcrag oxcrag commented Feb 22, 2020

No description provided.

@oxcrag oxcrag requested a review from mike-jumper February 22, 2020 23:25
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks okay to me - as with your PR on the client side:

  • Addition of the Makefile entry should be in (roughly) alphabetical order.
  • Pull Request and Commit Messages need the GUACAMOLE-968 tag on them.

@oxcrag oxcrag changed the title Added Norwegian keyboard map. GUACAMOLE-968: Add Norwegian keymap Feb 23, 2020
@oxcrag
Copy link
Author

oxcrag commented Feb 23, 2020

I fixed the entry order, renamed the pull request, and the commit message for the new version of the files. Is there a way of changing the previous commit message after the fact, or will that do bad things to the pull request/version management?

@necouchman
Copy link
Contributor

Is there a way of changing the previous commit message after the fact

Yes, you can do an interactive rebase, choose the option to modify the commit message, and then fix the messages. After you're done do a force push and it should be good.

@necouchman
Copy link
Contributor

Also note that the correct format for both PR (which you've done properly) and commit messages is "GUACAMOLE-968: ".

@oxcrag
Copy link
Author

oxcrag commented Feb 23, 2020

I'm (slowly) getting the hang of this; thanks again! I think the requested changes are in place now.

@oxcrag oxcrag requested a review from necouchman February 24, 2020 20:48
Copy link
Contributor

@mike-jumper mike-jumper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - jumped the gun there. The overall changes look good, but the commits should definitely be combined. If the commits embody distinct, logical changes, it would make sense to keep them, but their messages are identical:

Screenshot 2020-04-13 at 11 45 37 PM

@necouchman
Copy link
Contributor

This also may need to be updated, now, to take into account changes from #285 and #287 as part of GUACAMOLE-518.

@mike-jumper
Copy link
Contributor

Yep. Caps Lock will not behave correctly without the keymap dictating Caps Lock's behavior.

@necouchman
Copy link
Contributor

@oxcrag This still needs to be updated to take Caps Lock changes into account.

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.

3 participants