Feature/12711/user autocomplete #39

arpanfactual · 2018-06-06T21:26:24Z

📝 Description

Issue #12711

"merge master"

…Factual/kudos into feature/12711/user_autocomplete

joecox

Good work @arpanfactual!

A couple things in addition to comments:

You can remove the client/app/bundles/KudosApp/reducers/appReducer.jsx file committed here.
You can remove the root level package.json that is committed.
If you merge master into here, you'll get saraht's husky/prettier fix. That will get you some nice auto-formatting when you commit. It helps you adhere to the code style without having to do anything.

joecox · 2018-06-27T18:31:26Z

client/app/bundles/KudosApp/stores/ModalStore.js

+}
+
+const MODAL_STORE = new ModalStore()
+export default MODAL_STORE


I think there's not enough here for a new store. Can you put the action and observable in the AppStore?

joecox · 2018-06-27T18:33:40Z

client/app/bundles/KudosApp/models/AllUsers.js

+  }
+}
+
+export default AllUsers;


I don't think you need this model, I don't see that it adds anything over an Array.

joecox · 2018-06-27T18:34:25Z

client/app/bundles/KudosApp/stores/AppStore.js

@@ -18,13 +20,15 @@ class AppStore {
  @action
  loadClientData(props) {
    this.user = new User(props.user)
+    this.allUsers = new AllUsers(props.allUsers)


per my previous comment, this can just be an array. So assigning props.allUsers directly.

joecox · 2018-06-27T18:34:55Z

client/package-lock.json

@@ -16,7 +16,7 @@
    "@types/jss": {
      "version": "9.5.3",
      "resolved": "https://registry.npmjs.org/@types/jss/-/jss-9.5.3.tgz",
-      "integrity": "sha512-RQWhcpOVyIhGryKpnUyZARwsgmp+tB82O7c75lC4Tjbmr3hPiCnM1wc+pJipVEOsikYXW0IHgeiQzmxQXbnAIA==",
+      "integrity": "sha1-DBBt4/4LMkzUFz+sfasmwSzaYk4=",


Not sure why your local is using SHA1 but others are using SHA512. What version of node/npm are you running?

joecox · 2018-06-27T20:08:13Z

client/app/bundles/KudosApp/constants/appConstants.jsx

@@ -17,6 +17,9 @@ export const actionTypes = mirrorCreator([
  'FETCH_KUDOS_REQUEST',
  'FETCH_KUDOS_FAILURE',
  'FETCH_KUDOS_SUCCESS',
+  'MODAL_SWITCH',
+  'FETCH_ALL_EMAILS',
+  'FETCH_ALL_USERS'


I think this file is left over from the redux way, you can remove.

joecox · 2018-06-27T23:08:04Z

client/app/bundles/KudosApp/components/KudoModalRenderer.jsx

+  render() {
+    return(
+      <div className="give-kudo">
+        <button className="styled-kudo-button open-modal" onClick={this.modalClick.bind(this)}>


I think you can just pass the modalSwitch/toggleModal function here directly.

onClick={ AppStore.toggleModal }

Since modalSwitch/toggleModal doesn't do anything with arguments, it's fine to call it with the single event argument.

joecox · 2018-06-27T23:08:28Z

client/app/bundles/KudosApp/components/KudoModalRenderer.jsx

+        </button>
+        {MODAL_STORE.showModal ?
+          <GiveKudo showModal={MODAL_STORE.showModal}
+                    modalClick={this.modalClick}


You won't need this here either, since you can use AppStore.toggleModal straight up in GiveKudo.

joecox · 2018-06-27T23:12:06Z

client/app/bundles/KudosApp/components/GiveKudo.jsx

-    },
-  }).then(res => {
-    return res.data.map(obj => _.pick(obj, ['name', 'email']))
-  })


Since this is going away, you should also remove the action and route in Rails.

joecox · 2018-06-27T23:17:22Z

client/app/bundles/KudosApp/components/GiveKudo.jsx

-    return res.data.map(obj => _.pick(obj, ['name', 'email']))
-  })
+const getEmailFromUser = (userString) => {
+  //react-select gives values as name | email, so must parse email from that


Looking at the react-select docs, it seems like your onChange would get passed an array of objects, and thus you would avoid this issue, if you made simpleValue to be false. Have you tried that?

I think that simpleValue will give one long string that consists of all of the values returned which this function doesn't really deal with (this one deals with the actual content of each of the values) so each element in the array. This does bring up the point that I am splitting the string for no reason in my onchange function as I could just set simpleValue to true and get the array anyways

joecox · 2018-06-27T23:19:46Z

client/app/bundles/KudosApp/components/GiveKudo.jsx

@@ -67,98 +51,110 @@ export class GiveKudo extends React.Component {
      this.setState({ inFlight: true })
      AppStore.kudosStore.newKudo(this.state.emails, this.state.message)
      this.setState(this._initialState())
+      this.props.modalClick(e);


Here and below I think you can just use AppStore.toggleModal.

joecox · 2018-06-28T17:32:19Z

I got it running locally, looks really good!!

A couple usability things I noticed:

can the give kudo buttons have cursor: pointer applied?
the self-kudo easter egg is not centered anymore
when you're in the select component, I expect hitting 'tab' to put me into the next field (the message textarea), but it selects the top user from the select instead
can you make the modal not scroll with the page? I don't know if adds much usability, and it can run into a problem where you can't submit or close the modal if you expand the text area enough:

How difficult would it be to close modal on click outside of the modal?

arpanfactual · 2018-06-28T17:54:07Z

Yeah these are all great comments that I’ll look into; actually a lot of the clicking functionality and movement of the modal was fixed with that modalcover component and it’s styling so I gotta look into what happened there! Thanks again for goin through and catching all of this

arpanfactual · 2018-06-29T22:33:38Z

all the comments have been addressed but the package-lock changes! Not sure what's happening; if I take masters package-lock and then install react-select, everything changes.

Also, the tab issue is a bit more involved than i thought. It might not be something for this PR. The React Select owners have discussed it thoroughly and are still looking for a stand

but adjusting the value component was messing up styling for some reason. I'm definitely willing to look into it in another ticket!

JedWatson/react-select#643

joecox

Really awesome job @arpanfactual!

Thanks for looking into the react-select and package lock shenanigans, and great work with the usability!

Still seeing client/app/bundles/KudosApp/reducers/appReducer.jsx, can you remove?

For the package-lock, to me it seems like https://stackoverflow.com/a/47834854 is the culprit. I think you may have tried this fix given the npm in the package.json - are you running npm install inside or outside the container?

We can meet anytime if you'd like to look at anything together!

joecox · 2018-07-02T15:54:00Z

client/package.json

@@ -40,6 +40,7 @@
    "mirror-creator": "1.1.0",
    "mobx": "^4.3.1",
    "mobx-react": "^5.2.0",
+    "npm": "^6.1.0",


I think I saw this get removed from package-lock, but's let's remove from package.json too.

joecox · 2018-07-02T15:59:03Z

app/models/user.rb

+  #     hsh[u.name] = u.email
+  #   end
+  # end
+


Let's remove the commented out sections altogether.

joecox · 2018-07-02T16:01:19Z

client/app/bundles/KudosApp/components/GiveKudo.jsx

@@ -51,11 +30,11 @@ export class GiveKudo extends React.Component {
    // Uses lodash to bind all methods to the context of the object instance, otherwise
    // the methods defined here would not refer to the component's class, not the component
    // instance itself.
-    _.bindAll(this, 'handleClick', 'onChangeSearchInput', 'setMessage')
+    _.bindAll(this, 'handleClick', 'onSelectChange', 'setMessage', 'onSuccess', 'onFailure')


Interesting, not sure why this worked before this PR, since you added the _ lodash import. But let's make it consistent with the other imports: you can refer here to bindAll, and then import bindAll in the spread on line 1, along with isEmpty and trim.

joecox · 2018-07-02T16:14:16Z

client/app/bundles/KudosApp/components/KudoSelectMenu.jsx

+function constructOptions(user) {
+  const filterValue = `${user[0]} ${user[1]}`
+  const displayValue = `${user[0]} (${user[1]})`
+  return { value: filterValue, label: displayValue, email: user[1] }


Style tip: you can avoid the magic number-y array accesses here if you use an array destructure in the parameter list:

function constructOptions([ name, email ]) { ... }

Additional style tip: if you assign to value and label instead of filterValue and displayValue, you can use the object assignment es6 shorthand:

return { value, label, email }

Arpan Pal added 30 commits May 25, 2018 11:58

restyled modal

08492bf

Merge branch 'master' into feature/12710/give_kudos_redesign

5cef2a4

"merge master"

conditional rendering of modal

c5351e2

conditional rendering of form completed

5e4170b

restyled modal

23cb9ca

conditional rendering of modal

56f9b74

conditional rendering of form completed

e0bece0

new styling

4aea72d

modal completed without darkening background

4413b43

modal completed without darkening background

9411113

modal finished

ef04b66

cleaned up code

05216ba

emails are populated in the redux store

5f54a26

emails are populated in the redux store

302bd2f

ant-d setup done

8f04272

Merge branch 'feature/12711/user_autocomplete' of https://github.com/…

d67280a

…Factual/kudos into feature/12711/user_autocomplete

brought users to store

96404a0

autocomplete functionality finished (no select/multiselect)

ea4d347

multi-selection fully functional

f1fc657

restyled modal

79d2d27

conditional rendering of modal

801b171

conditional rendering of form completed

53e4e6b

modal completed without darkening background

b06e0d2

modal finished

38a1317

cleaned up code

c222efc

emails are populated in the redux store

b01ce6f

emails are populated in the redux store

2cb960a

ant-d setup done

49e12aa

brought users to store

f6ee9c9

autocomplete functionality finished (no select/multiselect)

ef7ef7b

Arpan Pal added 2 commits June 6, 2018 11:24

multi-selection fully functional

7f83015

feature completed

ebcd9e5

arpanfactual closed this Jun 6, 2018

restyled (correctly) autocomplete, revamped styling of modal

8c04aba

arpanfactual reopened this Jun 7, 2018

arpanfactual requested a review from trisarahsorus June 7, 2018 23:27

arpanfactual added 3 commits June 20, 2018 16:19

react-select added

9522d03

merged mobx

2c028b6

modal and autocomplete converted to mobx

243d6a9

arpanfactual requested review from joecox and arvin-foursquare June 21, 2018 18:16

restyling modal and removing unnecessary files

837464a

joecox suggested changes Jun 27, 2018

View reviewed changes

merge prettier

326d11e

arpanfactual closed this Jun 29, 2018

code style changes

3d6ecce

arpanfactual reopened this Jun 29, 2018

arpanfactual added 2 commits June 29, 2018 15:23

modal and user autocomplete box finished

034789a

package.lock managed

afc3326

joecox suggested changes Jul 2, 2018

View reviewed changes

arpanfactual added 3 commits July 2, 2018 11:38

style changes

5603928

package-lock sha1 to sha512

3e8fae1

self kudo recentered

185c547

joecox approved these changes Jul 2, 2018

View reviewed changes

arpanfactual merged commit 4af5f95 into master Jul 2, 2018

arpanfactual deleted the feature/12711/user_autocomplete branch July 2, 2018 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/12711/user autocomplete #39

Feature/12711/user autocomplete #39

arpanfactual commented Jun 6, 2018

joecox left a comment

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

joecox Jun 27, 2018

arpanfactual Jun 28, 2018

joecox Jun 27, 2018

joecox commented Jun 28, 2018

arpanfactual commented Jun 28, 2018

arpanfactual commented Jun 29, 2018

joecox left a comment

joecox Jul 2, 2018

joecox Jul 2, 2018

joecox Jul 2, 2018

joecox Jul 2, 2018

Feature/12711/user autocomplete #39

Feature/12711/user autocomplete #39

Conversation

arpanfactual commented Jun 6, 2018

📝 Description

joecox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joecox commented Jun 28, 2018

arpanfactual commented Jun 28, 2018

arpanfactual commented Jun 29, 2018

joecox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment