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-3443: Providing a means to show elements including those injected. #19

Merged
merged 14 commits into from Oct 31, 2018

Conversation

Projects
None yet
2 participants
@jobara
Copy link
Collaborator

commented Oct 11, 2018

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 11, 2018

@amb26 would you be able to review this PR?

gpii.simplify.set = function (that, state) {
var content = that.findContent();

if (!that.observer) {

This comment has been minimized.

Copy link
@amb26

amb26 Oct 15, 2018

Member

Why do we do this lazily rather than in an onCreate listener or some other determinable time in the workflow?

@@ -24,9 +24,10 @@
main: "main, [role~='main'], .main, #main",
genericContent: ".content, #content, .body:not('body'), #body:not('body')",
nav: "nav, [role~='navigation'], .navigation, .nav, #nav, #navigation",
search: "[role~='search'], [type~='search']"
search: "[role~='search'], [type~='search']",

This comment has been minimized.

Copy link
@amb26

amb26 Oct 15, 2018

Member

This whole component could deal with a good deal of docs explaining its purpose, strategy, what kind of contract it conforms to, etc. (right now it just appears to be a viewComponent - does it comply with any other requirements?)

This comment has been minimized.

Copy link
@amb26

amb26 Oct 15, 2018

Member

Methods, especially, such as "findContent" should have reasonable JSDocs, etc.

}]
}]
});

fluid.tests.simplifyTester.injectElm = function (that) {

This comment has been minimized.

Copy link
@amb26

amb26 Oct 15, 2018

Member

I don't think the abbreviation "Elm" for "Element" aids readability too much given the reader's first thought might be of trees : P

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 17, 2018

@amb26 I've refactored a couple of pieces out to grades. Ready for more review.

(function ($, fluid) {

/*
* `gpii.chrome.contentView` is a type of view component which extends the typical viewComponent faculties for

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

facilities?

This comment has been minimized.

Copy link
@jobara

jobara Oct 23, 2018

Author Collaborator

I meant "faculties" like "capabilities" but "facilities" seems good, so I'll switch to that.

selectorNames = fluid.makeArray(selectorNames);

var found = fluid.find_if(selectorNames, function (selector) {
var elms = that.locate(selector);

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

Odd stray space here - does this pass linting?

This comment has been minimized.

Copy link
@jobara

jobara Oct 23, 2018

Author Collaborator

Sadly it does pass linting. Thanks for catching it.

},
locateOutOfContent: {
funcName: "gpii.chrome.contentView.locateOutOfContent",
// the last argument isn't directly used, but is provided to force IoC resolution of the method which is

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

How is it that this function ends up being called before component creation is done?

This comment has been minimized.

Copy link
@jobara

jobara Oct 23, 2018

Author Collaborator

It's used in an expander of the gpii.simplify grade, which has gpii.chrome.contentView as a base grade. I suppose I could move the "{that}.locateInContent" argument into that expander instead. Please let me know what you think would be more appropriate.

content: ["article", "main", "genericContent"],
defaultContent: "",
members: {
content: {

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

Explain in a short comment what this member is used for

* Similar to `that.locate` but the search excludes any items found within the scope of the content.
*
* @param {Component} that - an instance of `gpii.chrome.contentView`
* @param {String} selectorName - a selector name as defined in the selector's block. Similar to using `that.locate`

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

selector's -> selectors

}
},
injectNavToggle: true,
components: {
observer: {
type: "gpii.chrome.mutationObserver",

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

Explain why we need a mutationObserver here. Given we rendered the content that we want to make visible ourselves, how did it become invisible?

This comment has been minimized.

Copy link
@jobara

jobara Oct 30, 2018

Author Collaborator

@amb26 I've tried to answer that in this comment at the top of the file https://github.com/jobara/gpii-chrome-extension/blob/GPII-3443/extension/src/content_scripts/simplification.js#L18-L26

Essentially because a parent element has visibility:hidden set, we need to explicitly set it to visibility:visible.

* node, the old one will be replaced. If an observation is disconnected, the observe method will need to be called
* again to re-instate the mutation observation.
*
* @param {Component} that - an instance of `gpii.chrome.mutationObserver

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

Missing closing `

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 23, 2018

@amb26 I've addressed the comments, and left some replies. Ready for more review.

@amb26 amb26 merged commit 8b5bf66 into GPII:master Oct 31, 2018

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Cheers @jobara - I've merged this since it looked good in itself, but there is a moderate amount of uncovered code in portBinding.js which I failed to catch during that work - e.g.
https://github.com/GPII/gpii-chrome-extension/blob/master/extension/src/lib/portBinding.js#L270
https://github.com/GPII/gpii-chrome-extension/blob/master/extension/src/lib/portBinding.js#L193
and also the error branch at https://github.com/GPII/gpii-chrome-extension/blob/master/extension/src/lib/portBinding.js#L230 is substantial enough that I think it should receive some coverage

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 31, 2018

@amb26 Thanks for catching that, I've filed it as https://issues.gpii.net/browse/GPII-3490

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.