Make ModulesSelect remain open after selection #655

eugenood · 2018-01-03T10:28:12Z

This PR is made with respect to #598.

I had implemented @yangshun original solution:

One solution I have is to retain the search term and results screen so that the Added label appears and the user will know that the operation was successful. I think it's ok to leave the search term and results there as people usually add modules from the same faculty anyway.

mods-bot · 2018-01-03T10:33:01Z

Deploy preview ready!

Built with commit 822b2ca

https://deploy-preview-655--nusmods.netlify.com

ZhangYiJiang

Thanks for the PR! Do take note of the bugs and see if you can fix them.

ZhangYiJiang · 2018-01-03T10:40:12Z

www/src/js/views/timetable/ModulesSelect.jsx

+  };
+
+  onStateChange = (changes: any) => {
+    if (Object.prototype.hasOwnProperty.call(changes, 'inputValue')) {


Just use _.has for this. Also, combine the two if statements

ZhangYiJiang · 2018-01-03T10:42:16Z

www/src/js/views/timetable/ModulesSelect.jsx

+    if (Object.prototype.hasOwnProperty.call(changes, 'inputValue')) {
+      if (!Object.prototype.hasOwnProperty.call(changes, 'selectedItem')) {
+        this.setState({
+          inputValue: changes.inputValue,


I see two bugs (not entirely sure if they are caused by this specific line, will investigate a bit further when I have time)

When blurring after selecting an item, this causes the input value to be set to the last select item's value (ie. it's module code), not the last input value

Cursor position is reset if you try to edit the middle of the search query

ZhangYiJiang · 2018-01-03T10:43:49Z

Oh, and ignore the CircleCI failure - that's because of a config error on our part I think

- Fix cursor jumping to the end of the input - Refactor `ModulesSelect`

eugenood · 2018-01-03T15:09:51Z

Here's what I changed in the latest commit 4e95fac:

Turns out the cursor issue is a known issue. I had fixed it accordingly.
According to downshift documentation, props of rendered element should be passed as arguments into the prop getter. I had changed it accordingly.
I refactored the class.

There is, however, a bug on mobile that I haven't figure out a fix. If you switch to numeric keyboard and type a digit, it will return back to the alpha keyboard.

ZhangYiJiang · 2018-01-03T15:20:20Z

www/src/js/views/timetable/ModulesSelect.jsx

  isOpen: boolean,
+  isModalOpen: boolean,
+  inputValue: string,
+  selectedItem: any,


This should probably match the type of item property that was passed into getItemProp, which is ModuleCode. Since it is nullable, this should be ?ModuleCode

yangshun · 2018-01-03T17:53:16Z

@nexolute Good stuff, thanks for the PR!

coveralls · 2018-01-07T13:07:06Z

Coverage increased (+0.6%) to 53.455% when pulling 7b04ffd on nexolute:modules-select-feedback into af40134 on nusmodifications:master.

…back

coveralls · 2018-01-08T06:27:44Z

Coverage decreased (-0.07%) to 52.786% when pulling 285e835 on nexolute:modules-select-feedback into af40134 on nusmodifications:master.

coveralls · 2018-01-08T08:13:15Z

Coverage decreased (-0.1%) to 52.716% when pulling 1f17e99 on nexolute:modules-select-feedback into af40134 on nusmodifications:master.

coveralls · 2018-01-08T11:51:28Z

Coverage decreased (-0.1%) to 52.716% when pulling 30a8d4b on nexolute:modules-select-feedback into af40134 on nusmodifications:master.

…back

coveralls · 2018-01-09T04:41:36Z

Coverage decreased (-0.1%) to 52.692% when pulling 0222523 on nexolute:modules-select-feedback into 43cdfe8 on nusmodifications:master.

coveralls · 2018-01-09T06:16:25Z

Coverage decreased (-0.1%) to 52.692% when pulling 9691dc0 on nexolute:modules-select-feedback into 43cdfe8 on nusmodifications:master.

coveralls · 2018-01-09T06:57:33Z

Coverage decreased (-0.1%) to 53.118% when pulling 5ce199d on nexolute:modules-select-feedback into 9091543 on nusmodifications:master.

taneliang

Could we add a background color for the dropdown? On iPad, you can scroll beyond the ends of the list and peek at the content behind it. This bug is present in master but we should probably just fix it here.

coveralls · 2018-01-09T10:04:13Z

Coverage decreased (-0.1%) to 53.118% when pulling 058995c on nexolute:modules-select-feedback into 9091543 on nusmodifications:master.

coveralls · 2018-01-09T10:10:12Z

Coverage decreased (-0.1%) to 53.151% when pulling 822b2ca on nexolute:modules-select-feedback into bcd198c on nusmodifications:master.

ZhangYiJiang · 2018-01-09T12:01:27Z

@nexolute Thanks for this PR, and sorry it took so long!

Make ModulesSelect remains open after selection

557172a

ZhangYiJiang requested changes Jan 3, 2018

View reviewed changes

ZhangYiJiang requested a review from li-kai January 3, 2018 10:44

Fix cursor jumping and refactor ModulesSelect

4e95fac

- Fix cursor jumping to the end of the input - Refactor `ModulesSelect`

ZhangYiJiang reviewed Jan 3, 2018

View reviewed changes

yangshun changed the title ~~Make ModulesSelect remains open after selection~~ Make ModulesSelect remain open after selection Jan 4, 2018

Fix modal opening by default

7b04ffd

eugenood force-pushed the modules-select-feedback branch from 1a01842 to 7b04ffd Compare January 7, 2018 13:05

Change selectedItem type to ModuleCode

8897161

eugenood force-pushed the modules-select-feedback branch from 39d2e3f to 8897161 Compare January 7, 2018 14:30

ZhangYiJiang added 2 commits January 8, 2018 14:12

Merge remote-tracking branch 'origin/master' into modules-select-feed…

aef62bd

…back

Remove unused ref and move Close button into Modal

285e835

Eugene Lim added 3 commits January 8, 2018 15:50

Fix incorrect isModalOpen state

619b2d4

Fix ModulesSelect not close after Escape is pressed

a26006f

Fix position of close button

1f17e99

Set default highlighted index

30a8d4b

ZhangYiJiang added 2 commits January 9, 2018 12:37

Minor tweaks

1948728

Merge remote-tracking branch 'origin/master' into modules-select-feed…

0222523

…back

ZhangYiJiang approved these changes Jan 9, 2018

View reviewed changes

nusmodifications deleted a comment from coveralls Jan 9, 2018

Revert template update and add comment on iOS bug

9691dc0

taneliang approved these changes Jan 9, 2018

View reviewed changes

Merge branch 'master' into modules-select-feedback

5ce199d

taneliang requested changes Jan 9, 2018

View reviewed changes

Add background color and replace styles with list-unstyled from BS

058995c

taneliang approved these changes Jan 9, 2018

View reviewed changes

Merge branch 'master' into modules-select-feedback

822b2ca

ZhangYiJiang merged commit b15733c into nusmodifications:master Jan 9, 2018

taneliang mentioned this pull request Jan 14, 2018

No feedback on successful addition of module to timetable on mobile #598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ModulesSelect remain open after selection #655

Make ModulesSelect remain open after selection #655

eugenood commented Jan 3, 2018

mods-bot commented Jan 3, 2018 •

edited

ZhangYiJiang left a comment

ZhangYiJiang Jan 3, 2018

ZhangYiJiang Jan 3, 2018

ZhangYiJiang commented Jan 3, 2018

eugenood commented Jan 3, 2018 •

edited

ZhangYiJiang Jan 3, 2018

yangshun commented Jan 3, 2018

coveralls commented Jan 7, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

taneliang left a comment •

edited

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

ZhangYiJiang commented Jan 9, 2018

Make ModulesSelect remain open after selection #655

Make ModulesSelect remain open after selection #655

Conversation

eugenood commented Jan 3, 2018

mods-bot commented Jan 3, 2018 • edited

ZhangYiJiang left a comment

Choose a reason for hiding this comment

ZhangYiJiang Jan 3, 2018

Choose a reason for hiding this comment

ZhangYiJiang Jan 3, 2018

Choose a reason for hiding this comment

ZhangYiJiang commented Jan 3, 2018

eugenood commented Jan 3, 2018 • edited

ZhangYiJiang Jan 3, 2018

Choose a reason for hiding this comment

yangshun commented Jan 3, 2018

coveralls commented Jan 7, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

taneliang left a comment • edited

Choose a reason for hiding this comment

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

ZhangYiJiang commented Jan 9, 2018

mods-bot commented Jan 3, 2018 •

edited

eugenood commented Jan 3, 2018 •

edited

taneliang left a comment •

edited