-
Notifications
You must be signed in to change notification settings - Fork 3
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 2452 Initial Suggestions and Review #1
Conversation
Temporarily including a local set of testData to ease development.
Committing default _settings.scss from Foundation 6.3.0 before making modifications.
- Removing markdown descriptions for now. - Changing "Add Pref Set" to "Add Context" - Removing context icons and earcons buttons since we don't have any yet. - Changed left hand product list to be <li>'s instead of <h4>'s
- Numerous changes based off of continuing prototyping, and the UI review of June.
- Removing half baked settings reference page
- Created utility routine for filtering an index - Updated usage in components - Added some unit tests for it
- Removing numerous console.log statements - Putting in a temporary fix for the add context dialog until it's refactored in to a proper component.
- Changed to Yes/No based on UI Review
- Renaming editprefset.js to editprefset-viewport.
- Setting up modelrelay from the main editPrefs component to the child product tables to save their search/filter settings in the event that page is rerendered, or we need to save all the settings.
@cindyli @amb26 I believe I'll addressed most of the feedback, and that the repo is hopefully in fairly maintainable shape now. There are a few comments that suggested improved decoupling between some of the components, but I think they will require a bit more work, so I would like to make jira tickets for them and mark them post 1.0 I do need to make a release of this for the project timeline, so if you're comfortable with this, I would like to merge what is currently here into the proper GPII repo and tag it. |
} | ||
}); | ||
|
||
gpii.devpmt.dialogs.confirmRemoveProductDialog.acceptConfirmDialog = function (closeDialog, prefsSet, product, editProductEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in other cases this simple sequential listener should be dismantled into two successive namespaced listeners with priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little bit of an edge case, and I have comments about it in the grade it inherits from. For certain dialogs that require this ordering, because of the way the Foundation close works, closing the dialog will stop the rest of the listener chain.
fluid.defaults("gpii.devpmt.editPrefs", { | ||
gradeNames: ["gpii.devpmt.viewComponent"], | ||
mergePolicy: { | ||
allSolutions: "noexpand", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is plenty wierd - how do values get into these options, and why mustn't they be expanded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are added by the gpii-handlebars helper tag that creates the component on the page template. They are listed as "noexpand", because some of the entries and syntax in them are interpreted as IoC references, variables, or other things during the component installation which tries to mangle them into something else.
Granted, there are a few other things that need to be included on a round of a ResourceLoader-isation, but I'd like to jira that and make it part of the next round of work.
@amb26 I've either resolved or added some dialog on the most recent comments. Any chance I can merge this in to the GPII/gpii-devpmt repo at this point? |
Tests are currently hanging. I get a bunch of failed calls to "parse" as follows:
and then finally the sequence hang message
|
I saw this as well and have fixed them up, and also added the note in the README on the current dependence on having a GPII-2966
|
@amb26 Having fixed up the existing tests, I'd like to potentially merge and tag this on Monday if you're feeling comfortable with it for now. |
I tested this with an instance of the GPII-2966 universal running on the local machine, and got just the same failure as listed above. I did notice that the universal instance got a connection on the new /prefssafes endpoint as below, so at least something is going right:
|
@amb26 Hi! I just cloned a fresh copy of both these branches and was able to run the tests successfully, as well as start it up and use the webapp in a browser. It looks above like maybe there is something wrong with the couch instance? In order to start up the GPII-2966 universal branch I did: #host machine
vagrant up
# in the fedora linux terminal
cd sync/universal
./scripts/vagrantCloudBasedContainers.sh I then changed the Virtual Box port forwarding to send 9081 -> 8081 |
@amb26 Ok, here are the exact steps I used to run this requiring zero docker or vagrant containers. (other than the fact that my local couchDB instance runs in a docker container, but any Couch available on 8081 should be fine). Obviously the remotes don't need to be renamed, but I wanted to paste the exact commands I used.
git clone [email protected]:GPII/universal.git
cd universal/
git remote rename origin GPII
git checkout -b GPII-2966 GPII/GPII-2966
npm install
GPII_COUCHDB_URL="http://localhost:5984/gpii" GPII_APP_DIR=$(pwd) bash -c ./scripts/deleteAndLoadSnapsets.sh
export NODE_ENV=gpii.config.preferencesServer.standalone.production
npm start
git clone [email protected]:sgithens/gpii-devpmt.git
cd gpii-devpmt/
git remote rename origin sgithens
git checkout -b GPII-2452 sgithens/GPII-2452
npm install
npm test |
Cheers - the test coverage of this pull is extremely bad (25% branch coverage) but I am merging since I have now been able to verify the tests |
This is the first PR for this repo. I realize there is a lot of work and mess that needs cleanup, but a first round of suggestions, especially for fluid and framework related stuff is appreciated.
https://issues.gpii.net/browse/GPII-2452