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

Starting the pull request process for PGA 0003 #108

Open
wants to merge 204 commits into
base: master
Choose a base branch
from

Conversation

benjaminstokes
Copy link

No description provided.

bjderzin and others added 30 commits January 15, 2016 16:15
changes from my local instance.
Removed internal style sheet from the prefsServerIntegration/index.html
Removed Preview Pane Button
Removed css refering to preview button
added internal style sheet to the main style sheet
commented the style sheet according to changes.
Removed Project file from git
…demos/index.html. Inline styles moved to stylesheet from confirmTemplate.html. navButtons.js, navIcons.js, firstDiscoveryEditor.js reverted to original. panels.js removed model listeners for logging, removed empty invokers, removed console log statements.Edits to congradulations.json messages for spanish and french. FriendlyWords functions changed for conmfirm panel in panels.js.
created css class for table settings.
firstDiscoveryPerfsServerInegration.html added css id: open to welcome
panel.
… an issue identified during integration testing
speech rate converted to words per minute
@@ -2,13 +2,13 @@
"bitwise": false,
"camelcase": false,
"curly": true,
"eqeqeq": true,
"eqeqeq": false,
Copy link
Member

Choose a reason for hiding this comment

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

These settings should be reverted to our project defaults - in particular the use of the === operator correponds to a JS best practice http://stackoverflow.com/questions/359494/does-it-matter-which-equals-operator-vs-i-use-in-javascript-comparisons#answer-359509

@mkbrenn
Copy link

mkbrenn commented Mar 18, 2016

@amb26

Hello Antranig,

Do the comments above represent a comprehensive review of all of the files in this pull request?

@amb26
Copy link
Member

amb26 commented Mar 18, 2016

Hi @mkbrenn - no, they don't. It's typical for code review to be an interactive process - especially when the contribution needs such significant rework, as this pull request does, it is very likely that addressing the first round of review comments will expose further issues that were not evident in the original form of the contribution.

@mkbrenn
Copy link

mkbrenn commented Mar 18, 2016

Thanks for the quick feedback @amb26. Are changes or feedback from the PGA team regarding the current comments, blockers to further review by the GPII team?

@amb26
Copy link
Member

amb26 commented Mar 18, 2016

Yes, we need to get these points addressed first, since they will generate a very large diff, create several new source files, linting changes, etc.

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.

6 participants