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

GPII-2429: Adding Character Spacing #7

Merged
merged 6 commits into from Apr 9, 2018

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Collaborator

commented Mar 13, 2018

Updated to the latest Infusion and added the Character Spacing adjuster and enactor.

Requires: GPII/universal#598

https://issues.gpii.net/browse/GPII-2429

@javihernandez

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

Hey @jobara,

after a quick review of the code, it looks good to me, but I'm having some problems to actually test the extension. When loaded on the browser, I can't get the UIO options popup to show and I get an infinite spinner instead. if I inspect the popup I can see:

jQuery.Deferred exception: Cannot read property 'preferenceMap' of undefined TypeError: Cannot read property 'preferenceMap' of undefined
    at Object.t.prefs.expandSchemaComponents (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:15179:175)
    at chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:15265:65
    at Object.t.each (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:6029:100)
    at t.prefs.expandSchema (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:15263:11)
    at t.invokeFunc (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:8399:11)
    at Object.t.expandExpander (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:8385:78)
    at Object.t.expandSource (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:8311:116)
    at Array.i (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:8327:26)
    at n (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:6603:61)
    at Array.<anonymous> (chrome-extension://ifommhafkphfognacgcmpidnpbffedlp/dist/ui-options-chrome-adjustersLib.min.js:7247:24) undefined

Uncaught TypeError: Cannot read property 'preferenceMap' of undefined
    at Object.t.prefs.expandSchemaComponents (ui-options-chrome-adjustersLib.min.js:15179)
    at ui-options-chrome-adjustersLib.min.js:15265
    at Object.t.each (ui-options-chrome-adjustersLib.min.js:6029)
    at t.prefs.expandSchema (ui-options-chrome-adjustersLib.min.js:15263)
    at t.invokeFunc (ui-options-chrome-adjustersLib.min.js:8399)
    at Object.t.expandExpander (ui-options-chrome-adjustersLib.min.js:8385)
    at Object.t.expandSource (ui-options-chrome-adjustersLib.min.js:8311)
    at Array.i (ui-options-chrome-adjustersLib.min.js:8327)
    at n (ui-options-chrome-adjustersLib.min.js:6603)
    at Array.<anonymous> (ui-options-chrome-adjustersLib.min.js:7247)

Can you take a look at this? Thanks!!

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2018

@javihernandez i can't seem to reproduce this. Could you please let me know which version of Chrome you are using. I just tested this again on Chrome 65.0.3325.181 in macOS 10.13.3 and was able to load it without issue. Also did you run grunt build or grunt buildDev before loading the unpacked extension into Chrome?

@javihernandez

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Hey,

yes, I've ran grunt build and buildDev just in case, had the same problem.
I'm on Chrome Version 60.0.3112.90 (Official Build) (64-bit) in GNU/Linux (openSUSE tumbleweed, the openSUSE's rolling distro).

Let me check on Windows.

@javihernandez

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Interesting, it works on Windows but can't get it working on GNU/Linux after upgrading Chrome to its latest stable release, 65.0.3325.181. Then I'd say that it could be an OS-specific problem, but why? Do you have any ideas?

@javihernandez

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

I can't find any explanation for this problem on GNU/Linux, but it's very weird the fact that master works fine and I'm still getting such error even after updated to the last version of Chrome.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2018

@javihernandez, yesterday I setup an Ubuntu vm and also encountered the error there. I'll do some debugging today to see if I can determine what the cause is.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2018

@javihernandez, I found the problem. I had a typo where the paths to some of the letter space resources were spelled "letterSpace" instead of "LetterSpace". Because linux is case sensitive, it failed to find these files, but worked on the other platforms that aren't. It should all be fixed up now and ready for more review.

@javihernandez

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Okay, I knew it could be a stupid typo but I couldn't find it myself, thanks!!
I've just tested it on GNU/Linux now and it works perfect too :)
@amb26 this looks good to me and the universal side is already merged in. Let me know if you want to give a quick look at it before proceeding with the merge.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2018

I think 90% of my coding issues are typos

@amb26

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

lgtm

@javihernandez javihernandez merged commit 05c24cf into GPII:master Apr 9, 2018

@jobara jobara deleted the jobara:GPII-2429 branch Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.