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

edit perspectives #236

Merged
merged 6 commits into from
Feb 10, 2017
Merged

edit perspectives #236

merged 6 commits into from
Feb 10, 2017

Conversation

annyhe
Copy link
Contributor

@annyhe annyhe commented Feb 3, 2017

No description provided.

@annyhe annyhe force-pushed the mainPersBr branch 2 times, most recently from 734dd1d to 56be1f1 Compare February 3, 2017 18:33
@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-236 February 3, 2017 18:35 Inactive
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.266% when pulling 56be1f1 on mainPersBr into 6355731 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.266% when pulling 56be1f1 on mainPersBr into 6355731 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.266% when pulling 56be1f1 on mainPersBr into 6355731 on master.

@annyhe annyhe changed the title DO NOT MERGE edit perspective edit perspectives Feb 8, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling eb707aa on mainPersBr into a7d599d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling eb707aa on mainPersBr into a7d599d on master.

iamigo
iamigo previously requested changes Feb 9, 2017
Copy link
Contributor

@iamigo iamigo left a comment

Choose a reason for hiding this comment

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

Can we get it look at it in a review app?

expect(instance.state.dropdownConfig.subjects.options.length).to.equal(ZERO);
});

it.skip('field subject tag filter array is populated from props',
Copy link
Contributor

Choose a reason for hiding this comment

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

skip?

@@ -0,0 +1,31 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

needs copyright header block


module.exports = {
getSubjects,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

needs trailing carriage return

@annyhe annyhe force-pushed the mainPersBr branch 2 times, most recently from a03b369 to 8febbc9 Compare February 9, 2017 00:44
@annyhe annyhe dismissed iamigo’s stale review February 9, 2017 00:44

implemented changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 3915af8 on mainPersBr into a7d599d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 3915af8 on mainPersBr into a7d599d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 3915af8 on mainPersBr into a7d599d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 3915af8 on mainPersBr into a7d599d on master.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 9, 2017

@iamigo I deployed a review app just now. Takes a few minutes to setup and push data to it.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 9, 2017

Copy link
Contributor

@iamigo iamigo left a comment

Choose a reason for hiding this comment

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

working very nicely!

laf doesn't seem fully "lightning-y" -- I can explain better in person or on a hangout--let's go over it together tom'w

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 74fa9b0 on mainPersBr into a7d599d on master.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 9, 2017

travis CI choked on api test: might be a flapping test as none of the changes affect the API. Will do another PR after going over the UI changes with Ian, and the tests should pass.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.177% when pulling 120e7d4 on mainPersBr into 2f27d36 on master.

@pallavi2209
Copy link
Contributor

@annyhe I cant see Edit/pencil option on review app. Let me know once this is deployed, I would like to playaround :)

@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-236 February 9, 2017 21:58 Inactive
@annyhe annyhe temporarily deployed to refocus-pr-236 February 9, 2017 21:59 Inactive
@annyhe
Copy link
Contributor Author

annyhe commented Feb 9, 2017

@pallavi2209 I am deploying to a new review app. The old one doesn't accept new commits.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 360616b on mainPersBr into 9a8a8c2 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 360616b on mainPersBr into 9a8a8c2 on master.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 10, 2017

Updated as per Ian's UI comments
screen shot 2017-02-10 at 9 53 50 am
screen shot 2017-02-10 at 9 51 46 am

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 83b9ade on mainPersBr into 9a8a8c2 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 7de39ac on mainPersBr into 9a8a8c2 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 81279a7 on mainPersBr into 9a8a8c2 on master.

@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-236 February 10, 2017 21:06 Inactive
@annyhe
Copy link
Contributor Author

annyhe commented Feb 10, 2017

DO NOT merge: the css on https://www.lightningdesignsystem.com/components/menus/#flavor-dropdown-dropdown-menu-icon-double differs from the one we use, so the dropdown styles are messed up. Will update when it gets fixed.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 10, 2017

@iamigo @pallavi2209 The css mess is fixed. Please review demo app http://refocus-pr-236.herokuapp.com/perspectives/sdsdsds

@@ -179,7 +179,7 @@ class Dropdown extends React.Component {
let outputUL = '';
// if options exist, load them
if (data.length) {
outputUL = <ul className="slds-dropdown__list" role="menu">
outputUL = <ul className="slds-lookup__list" role="menu">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This css class is essential to have the two column effect. Otherwise the icon, text, and icon on the right all float to the left.
screen shot 2017-02-10 at 9 51 46 am

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the "New Perspective" label is not aligned with the names of the perspectives--it looks a few pixels to the right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that easy to tweak? If not, I'm ready to move on :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we move on? If it really bothers people we can fix it later.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 98a7a9e on mainPersBr into 9a8a8c2 on master.

@pallavi2209
Copy link
Contributor

pallavi2209 commented Feb 10, 2017

So, when I am on /perspective page, and I delete/add some subjects, and then I try and edit a perspective, should I get updated list of subjects in Root subject dropdown? @annyhe @iamigo

Other than that, the review app looks good.

@annyhe
Copy link
Contributor Author

annyhe commented Feb 10, 2017

@pallavi2209 After refreshing the page you ought to get the list of updated subjects. subjects are set on page load.

@pallavi2209 pallavi2209 merged commit 05779c0 into master Feb 10, 2017
@pallavi2209 pallavi2209 deleted the mainPersBr branch February 10, 2017 23:10
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

5 participants