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

Tentative support for options parameter, including icon color and glyph #16

Closed
wants to merge 5 commits into from

Conversation

gcordalis
Copy link
Contributor

@gcordalis gcordalis commented Nov 26, 2018

Added basic support for options parameters to accommodate custom icon colours and glyphs.

I've included a new meta directory which includes shortcutColors and shortcutGlyphs... Colours have been named based on appearance, there aren't names in the font file (that I could find) so they're labelled based on their unicode hex value.

I haven't created a reference to either of these files, TS wasn't doing what I expected it to on import, I've tried a module declaration in src/meta to no avail.

Being completely honest, I've got no idea what I need to adjust with the tests. To test this worked I temporarily renamed the __tests__ directory, built using tsc --build tsconfig.json and tested using the script below.

This produced a functional shortcut with the correct colour and glyph specified.

const fs = require("fs");

const { buildShortcut, withVariables } = require("./utils");

const { calculate, comment, number, showResult } = require("./actions");

// We'll use this later to reference the output of a calculation
let calcId;

// Define our list of actions
const actions = [
  comment({
    text: "Hello, world!"
  }),
  number({
    number: 42
  }),
  calculate(
    {
      operand: 3,
      operation: "/"
    },
    id => {
      // We'll use this again in a moment
      calcId = id;
    }
  ),
  showResult({
    /**
     * We can use the result of the calculation in this Shortcuts's input
     * by passing the string to the 'withVariables' tag function
     */
    text: withVariables`Total is ${calcId}!`
  })
];

const options = {
  icon: {
    color: 4274264319,
    glyph: 59447
  }
};

// Generate the Shortcut data
const shortcut = buildShortcut(actions, options);

// Write the Shortcut to a file in the current directory
fs.writeFile("optionsTest.shortcut", shortcut, err => {
  if (err) {
    console.error("Something went wrong :(", err);
    return;
  }
  console.log("Shortcut created!");
});

@joshfarrant
Copy link
Owner

This is a great start, thanks! I'll take a look at writing the tests for you and I'll try to figure out what TS is doing too, so don't worry about that.

One thing to just be aware of is that it looks like your text editor is maybe running an auto-fix lint on save with an incorrect (maybe global?) linter config. That's caused a few additional changes to be shown in the diff. For example, if you take a look at the buildShortcutTemplate.ts diff you'll see that single quotes have been changed to double quotes, and that trailing commas have been removed, along with a couple of other things.

To fix this, you should be able to download a tslint plugin for your editor (VSCode, Atom), which should automatically detect the tslint.json and show you what needs to be fixed. If that doesn't work, you can just run npm run lint from the root of the project and you should see the errors printed in your terminal. 🙂

@gcordalis
Copy link
Contributor Author

Back to sublime I go... I’ll fix up the linting and resubmit to avoid all the unnecessary changes

Double quotes to single quotes
Reverted vscode auto-linting
Reverted auto-linking from vscode
@xAlien95
Copy link
Contributor

@gcordalis, this pull request is currently a bit messy and it probably won't be merged also after lint and test corrections.

I would like to help you setting up a new fork and a new pull request. Can you contact me on Telegram (my username is the same I have here on GitHub)? I'll guide you on how to properly install and configure Atom and I'll give you some tips on how to correctly provide your additions in TypeScript.

@joshfarrant
Copy link
Owner

@xAlien95 I started improving the few outstanding bits on this PR over the weekend, and I've got it all in a local branch. Happy to push that branch this evening if you both want to take a look. I've just got a couple more tests to write iirc before it's good to go.

@joshfarrant
Copy link
Owner

@xAlien95 @gcordalis I've just pushed my modifications to the PR to the branch pr/16 (see 6728d85).

I modified the way default actions are set, and restructured your colours and glyphs files. The changes also necessitated modifications to the buildShortcut and buildShortcutTemplate tests, so I've included those in this branch too.

Let me know if either of you have any feedback on this. I'll merge it in to master once you've both had change to look.

@gcordalis There are probably too many changes to reasonably expect you to modify your PR and re-implement them, so I'd just suggest taking a look at the changes I made and seeing if you can understand why I made them. Don't worry about fully understanding the tests or TypeScript interfaces if they aren't something you're familiar with, just take a look at the modifications I made to the files you added. I'm happy to answer any questions you have on any of the decisions I made!

@xAlien95
Copy link
Contributor

xAlien95 commented Nov 28, 2018

@joshfarrant, awesome!

May I suggest to use hex numbers for colors? You'll se what those numbers actually are:

export const COLORS = {
   RED: 4282601983,
   DARK_ORANGE: 4251333119,
   ORANGE: 4271458815,
   YELLOW: 4274264319,
   GREEN: 4292093695,
   TEAL: 431817727,
   LIGHT_BLUE: 1440408063,
   BLUE: 463140863,
   DARK_BLUE: 946986751,
   DARK_PURPLE: 2071128575,
   LIGHT_PURPLE: 3679049983,
   PINK: 3980825855,
   DARK_GRAY: 255,
   GRAY: 3031607807,
   BLUE_GRAY: 2846468607,
 };
export const COLORS = {
   RED: 0xFF4351FF,
   DARK_ORANGE: 0xFD6631FF,
   ORANGE: 0xFE9949FF,
   YELLOW: 0xFEC418FF,
   GREEN: 0xFFD426FF,
   TEAL: 0x19BD03FF,
   LIGHT_BLUE: 0x55DAE1FF,
   BLUE: 0x1B9AF7FF,
   DARK_BLUE: 0x3871DEFF,
   DARK_PURPLE: 0x7B72E9FF,
   LIGHT_PURPLE: 0xDB49D8FF,
   PINK: 0xED4694FF,
   DARK_GRAY: 0x000000FF,
   GRAY: 0xB4B2A9FF,
   BLUE_GRAY: 0xA9A9A9FF,
};

They are simply RGBA-8 stored colors (alpha is always 255, I wonder if Shortcuts can handle custom colors.. 🤔).

EDIT: src/meta/glyphs.ts has more icons than needed (if I counted correctly there should be 227 icons in total in the app), someone has to rename each one into .ROCKET, .KEYBOARD and also check if the glyphs is actually in the icon list.. a lot of work to do!

I'm also following your tests with TypeDoc (I tried searching something too, I tried compodoc, but it builds docs for all the Interfaces and it handles all the actions as constants since in their .ts files are actually consts and not functions).
It could be useful to have in the docs a grid with all the icons and the names (.ROCKET, .KEYBOARD, ...) below (similarly to ionicons, if hosting those icons is legally possible - maybe in rasterized .pngs?), but for such a feature docs have to be written manually. Manually written docs may be a better solution for this node module, since only actions, tutorials and examples have to be probably written. Your thoughts?

@joshfarrant
Copy link
Owner

I wonder if Shortcuts can handle custom colors.. 🤔

Sadly not as far as I can tell. It was one of the first things I tried changing when I got the proof-of-concept working!

@gcordalis
Copy link
Contributor Author

This looks great, I vaguely see what you've done with tests but it's not something I've ever looked in to so a lot is over my head.

I haven't been successful in setting custom colours either, which I found a bit surprising tbh.

May I suggest to use hex numbers for colors?
I think this makes a lot of sense for this repo. I handle these as RGB on my end and put forward the base colour found in workflow to normalise it a bit.

@joshfarrant In the changes to buildShortcut does the ? in options imply that it's an optional param included when the buildShortcut function is called?
You've then set a hard value if there isn't an options param, and take the param value if it is there?

I'll start testing every glyph in app this morning and will remove the ones that aren't in use before you commit this to master.

@gcordalis
Copy link
Contributor Author

All of the glyphs load in the app (except for the old company logos that default to another icon).

There are a lot of double ups though...

The closest thing to a pattern I've found is that E8XX glyphs have been updated and included from E9XX.

Removing all E8XX glyphs leaves us at 223 total, which is pretty close to the 227 you got to @xAlien95

@joshfarrant
Copy link
Owner

In the changes to buildShortcut does the ? in options imply that it's an optional param included when the buildShortcut function is called?
You've then set a hard value if there isn't an options param, and take the param value if it is there?

Yes, the ? indicates to TypeScript that the given parameter might not always be included. This allows the compiler to check our code and make sure we're not just blindly assuming that it will always exist.

Great work going through the list of icons, hope you found a way to automate the process slightly! It's a shame there's no standard list of names for the icons anywhere. It's a custom set of icons, I'm assuming, which have stayed mostly the same since Workflow days?

@xAlien95
Copy link
Contributor

xAlien95 commented Nov 28, 2018

It's a shame there's no standard list of names for the icons anywhere.

I started to list all the glyphs..

The "People" tab is complete.
The "Symbols" tab is complete.

I'm at 98% of total, but I'm using the glyphs extracted from the old Workflow app (I'm unable to extract them from the last update of Shortcuts app).

@gcordalis
Copy link
Contributor Author

I've just added a PR #17 to the new branch removing some of the duplicates or redundant icons.

I'm going to take a break for a while and will do another pass with fresh eyes.

Unfortunately, there isn't a hugely automated way...
I generated a shortcut for each glyph, imported them all to my iPad (I was shocked that shortcuts handled importing 352 shortcuts in one hit without issue!!!) and compared the icons displayed in app to my web editor which had the raw SVGs... Not particularly difficult, just tedious and room for human error.

I didn't test too thoroughly, but noticed that 12345 generates a blank glyph in app.

@xAlien95
Copy link
Contributor

It took me three hours but now I have them all listed (in Italian) in the same order of Shortcuts app. You can render the list downloading the .html I put in this gist (check the css to get the icons). I also added a glyphs.ts with all the unicode hex numbers in the right order. Since I'm not a native English speaker, I prefer not to decide all the icon names, so someone with some spare time should fill the .ts file with all the keys (.ROCKET, .KEYBOARD, and so on).

@gcordalis, Shortcuts glyphs are exactly 256, I counted them wrong: 158 in objects tab, 21 in people tab and 77 in symbols tab.

@gcordalis
Copy link
Contributor Author

Thank you for the order. This is amazing!

Sent with GitHawk

@joshfarrant
Copy link
Owner

@gcordalis Let me know when you're happy for this to be merged 👍

@gcordalis
Copy link
Contributor Author

Let’s do it!

@gcordalis
Copy link
Contributor Author

@joshfarrant Just pinging for visibility.

Also, can not sure when you're planning to, but once you've merged this could you please publish to npm :)

@joshfarrant
Copy link
Owner

joshfarrant commented Dec 4, 2018

Not sure why merging didn't automatically close this PR, but this was merged in 694f445.

I'll publish a new version to npm shortly.

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

3 participants