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

[winchus] Moving Winchus keyboard to release after revamping documentation. #3354

Merged
merged 15 commits into from
Feb 27, 2025

Conversation

alex-larkin
Copy link
Contributor

  • /experimental/w/winchus/ deleted and /release/w/winchus/ created
  • Welcome.htm, winchus.php, and readme.md completely revised and reformatted.
  • Some other small tweaks and revisions made to keyboard such as showing diacritics on [K_COLON] on the onscreen keyboard, and a flick correction to the symbols layer of the touch layout. Winchus.kps package details description updated.

Could the reviewer please check if my links to winchus.pdf (in winchus/source/help/) work in welcome.htm, winchus.php, and readme.md? Also, if my (#english) anchor link and comments in readme.md are working as expected.

I welcome any feedback and will be happy to make any needed corrections.

Thanks so much!

Moving this keyboard to the release folder
welcome.htm and winchus.php completely revised and reformatted. Some other small tweaks and revisions made to keyboard such as showing diacritics on [K_COLON] on the onscreen keyboard, and a flick correction to the symbols layer of the touch layout.
deleting to reupload because i made a lot of changes to the documentation and related files.
welcome.htm, winchus.php, and readme.md completely revised and reformatted. Some other small tweaks and revisions made to keyboard such as showing diacritics on [K_COLON] on the onscreen keyboard, and a flick correction to the symbols layer of the touch layout. Winchus.kps package details description updated.
@keyman-server
Copy link
Collaborator

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

@alex-larkin
Copy link
Contributor Author

Ready for review. Also, I meant to ask if the reviewer could please check if my Winchus.kps package details description is looking okay. Thanks!

@LornaSIL LornaSIL changed the title Moving Winchus keyboard to release after revamping documentation. [winchus] Moving Winchus keyboard to release after revamping documentation. Feb 20, 2025
Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

Some missing files and broken links

<FileType>.htm</FileType>
</File>
</Files>
<Keyboards/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The keyboard package fails to build because you need to add the winchus keyboard to it.
In Keyman Developer, go to the package editor to add the keyboard.

Remember to also assign the Quechua language to the touch layout keyboard (.js)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Just to be sure, what's the extension I should look for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add the files in the build/ folder:

  • winchus.kmx
  • winchus.js

<a href="winchus.pdf" style="text-decoration: none;">
<div style="border: 1px solid black; padding: 10px; display: inline-block; text-align: center; background-color: #f2f2f2;">
<b>Descarga este documento<br />para leerlo sin conexión</b>
<img src="file-type-pdf-47.png" style="display: block; margin-left: auto; margin-right: auto; padding-top: 10px;" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all the assets for the php file are in the images/ folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Should I move the images up a level or change the link to images/filename.png?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can leave the images and update all the links

@alex-larkin
Copy link
Contributor Author

alex-larkin commented Feb 23, 2025

Thanks so much! I'll take a look at this on Monday afternoon Peru time.

@DavidLRowe
Copy link
Contributor

We would encourage you to remove the version number and date from the README.md file in order that, when/if the keyboard is updated, this README.md file will not need modification.

On Monday I'll try to look at the images, files and folders. I think when you create the PDF file it includes all referenced images, so those images don't need to be available for the PDF file to display properly. In contrast, the .php file (the online help for the keyboard) and the welcome.htm file (the help included with the keyboard package) need to have any referenced images available. For the .php file, that means they need to be in the "help" folder. For the welcome.htm file, they need to be included in the package's file list (in the .kps file).

Currently the package (.kps file, Keyboards tab) has no BCP 47 codes defined.
On the Details tab, please select welcome.htm for the Welcome file and readme.htm for the Readme file.

Made changes requested in PR keymanapp#3354 in keymanapp/keyboards: 
* Pulled images out of /images directory and up into /help. 
* Copied winchus.pdf
* Added winchus.kmx and winchus.js
@alex-larkin
Copy link
Contributor Author

alex-larkin commented Feb 26, 2025

Hi there!

Thanks for all of the valuable feedback. I'm sorry these updates took a little longer than I had hoped.

I made the changes requested:

  • Pulled images out of /images folder and up into /help. (Is that okay? It will make updating the winchus.PHP from the welcome.htm a lot easier)
  • Copied winchus.pdf over
  • Added winchus.kmx and winchus.js
  • I added the "qu" language tag to the keyboard

I also spell-checked my comments in Winchus.kmn

How is it looking?

@DavidLRowe
Copy link
Contributor

Pulled images out of /images folder and up into /help. (Is that okay? It will make updating the winchus.PHP from the welcome.htm a lot easier)

More that just "okay", I think moving the images out of the "images" folder into the "help" folder is the best solution for your very detailed documentation! However, from a quick look it seems that this change is not yet completed:

  • It's true that the .php file (from what I can see) no longer refers to the "images" folder
  • however the files themselves are still in the "images" folder and need to be moved.

You may need to delete all the files in the "images" folder on GitHub, then copy them from your computer to the "help" folder on GitHub. Once you've done that, GitHub will figure out that you're really doing a move.

@DavidLRowe
Copy link
Contributor

It looks as though the welcome.htm references the winchus.pdf file, so a copy of that file is needed in the "welcome" folder (where the package's file list correctly thinks it should be).

(The welcome.htm file and the files it references provide the help that is packaged with the keyboard. The winchus.php file and the files it references provide the help that is available online. A future enhancement should allow set of source files to provide both types of help files, but until then we need to have duplicate copies of the information.)

When I try to compile the package, it complains that there is no keyboard file and, indeed, in the "Files" list there is no reference to .kmx, .kvk or .js files. Those can be added by using the "Add..." button on that page. In addition, on the "Keyboards" tab, you should add one or more BCP 47 codes corresponding to languages the keyboard supports.

I wonder if the changes you noted above were actually made or are waiting to be completed?

@alex-larkin
Copy link
Contributor Author

🤔 Hmn, this is my first time trying to update a PR. In my fork all the files are as they should be, and when I committed then I saw it updated here:

image

Thoughts? Do I need to do something different?

Thanks!

@DavidLRowe
Copy link
Contributor

Unfortunately, your new changes ended up in the wrong place. (Sadly, I can attest that this is very easy to do!) Instead of being a sibling folder of "release", "winchus" needs to be a child folder of "w" which is a child folder of "release".
image

How to fix this?
(First a side note: The recommended way to work is to create a "branch" (called whatever you like) where you place your changes and then the pull request is based on that. Any updates to that branch become part of the pull request. If you decide the pull request is in error or not needed, you can delete the branch with no consequences. In this case you've based the pull request on your "master" branch, so this option is not available, short of closing this PR, deleting your fork, reforking the keyboards repository and creating a new pull request, which would be perfectly fine if you think that would be easier.)

You need to delete the "winchus" folder that is at the root (sibling of "release") along with all the files and folders under it. Then you'll need to redo the changes you did above. If this isn't enough direction please write again.

@DavidLRowe
Copy link
Contributor

Alternatively, I could take your files and update the keyboard on your behalf. (You'd still want to clean up your fork and it would likely be easier to to delete the existing fork and create a new one.)

uploaded to wrong spot...
deleted /winchus from keymanapps/keyboards and reuploaded to keymanapps/keyboards/release/w/. Let's see if this works!
@alex-larkin
Copy link
Contributor Author

Thank you David for helping me to sort that out.

It looks like when I tried reuploading the /winchus folder, I dropped it in my forked root by mistake, instead of /w as you said.

I deleted an extra branch from my fork, deleted /winchus from keymanapps/keyboards and reuploaded to keymanapps/keyboards/release/w/.

Then i deleted the /images folder found in /help.

I think that now everything is where it should be. Please let me know how it's looking!

Thanks so much.

border-bottom: 0px solid #B3B1B1;
border-radius: 25px;
color: #000000;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a stray closing brace that needs to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's part of the .kbd-resultado class but was indented wrong. I fixed it.

Comment on lines 8 to 244
vertical-align: middle; /* Vertically center */
min-width: 30px; /* Ensure all cells have the same width */
}

/* @page, @page landscape-secion, y .landscape permiten los teclados en la sección de "Configuración del teclado" imprimirse en modo horizontal, lo cual es importante al imprimir en A5 (lo cual facilita la lectura en celulares). */
@page {
size: auto;
}

@page landscape-section {
size: landscape;
}

.landscape {
page: landscape-section;
}

/* INICIO DE CLASES PARA MOSTRAR EL TECLADO*/
/* COPIADO Y PEGADO DESDE EL ARCHIVO GENERADO POR KEYMAN DEVELOPER */
/* winchus.kmn , On-Screen , Export... */

.key {
float: left;
display: block;
position: relative;
overflow: hidden;
height: 35px;

margin: 2px 0px 0px 4px;

}

#K_SPACE {
width: 234px;

background-image: url('key-space.gif');

}

#K_BKSLASH {
width: 42px;

background-image: url('key-bkslash.gif');

}

#K_oE2 {

display:none;

}

#K_SHIFTL {
width: 80px;background-image: url('key-shiftl.gif');
}

.plain {

background-image: url('key-plain.gif');

background-repeat: no-repeat;
width: 34px;
}

.special {
background-repeat: no-repeat;

}

.keycap {
font: bold 7pt Arial;
position: absolute;
left: 6px;
top: 6px;
}

.key .keycap {
display: none;
}

.special .keycap {
display: block;
}

.keytext {

font:
12pt

"Arial";
position: absolute;
display: block;
right: 5px;
bottom: 4px;
color: blue;
}

/* FIN DE CLASES PARA MOSTRAR EL TECLADO*/

END;
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire CSS block needs to go inside the <?php block near the top.

An example is arbore.php

<?php
  $pagename = 'Arbore Keyboard Help';
  $pagetitle = $pagename;
  require_once('header.php');

  $pagestyle = <<<END
    .coldiv { display: flex; flex-direction: row; gap: 5%;}
    table, tr, th, td { font: 10pt Arial; border-collapse: collapse; border: solid 1px; padding: 5px 5px; text-align: center;}
    th { font-weight: bold;}
    .combo { font-family: Courier, monospace; padding: 0px 10px;}
    .empty { background-color: lightgray;}
  END;

?>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Maybe this could be clarified at https://help.keyman.com/developer/keyboards/phphelpfile ? Or maybe I could suggest an edit directly later today.

$pagetitle = $pagename;
require_once('header.php');
?>
<meta name="viewport" content="width=device-width, initial-scale=1.0"><title>Winchus</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

The <title>Winchus</title> can be removed since it's handled by $pagetitle above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Maybe this could be clarified at https://help.keyman.com/developer/keyboards/phphelpfile ? Or maybe I could suggest an edit directly later today.

@DavidLRowe
Copy link
Contributor

@alex-larkin The files and folders all look like they are in the right places. Congratulations! Please review the comments that @darcywong00 made. He's more familiar with the PHP format than I am, so I value his review. I think with those changes this pull request should be ready to merge.

I also removed duplicate CCS section.
@alex-larkin
Copy link
Contributor Author

Thanks so much David and Darcy!

I have made the requested edits.

I also removed a duplicate CCS section.

?>
<meta name="viewport" content="width=device-width, initial-scale=1.0"><title>Winchus</title>

<meta name="viewport" content="width=device-width, initial-scale=1.0">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this line too. (It's HTML that doesn't belong in the PHP block).

@alex-larkin
Copy link
Contributor Author

Ok, thanks. I just did that.

@DavidLRowe DavidLRowe dismissed darcywong00’s stale review February 27, 2025 18:38

requested change was accepted

@alex-larkin
Copy link
Contributor Author

🥳 Thanks so much David and Darcy!

@DavidLRowe DavidLRowe merged commit e520ff3 into keymanapp:master Feb 27, 2025
2 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.

4 participants