Display module title in timetable cell #709

shanwpf · 2018-01-12T22:17:47Z

Solves issue #677

Added a module title button beside the orientation button.
Had to make changes to the timetable actions button layout to accommodate the new button.
In vertical mode, module titles are not displayed and the button is disabled.

Screenshots

Horizontal mode single row

Horizontal mode 2 rows

Vertical mode

mods-bot · 2018-01-12T22:39:05Z

Deploy preview for nusmods ready!

Built with commit bda98cc

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

taneliang

Thanks for the PR! We have been getting many requests for this.

One small UI bug: the button texts overflow in vertical mode on portrait iPad.

taneliang · 2018-01-13T02:25:32Z

www/src/js/reducers/timetables.js

@@ -125,10 +127,11 @@ export const defaultTimetableState: TimetablesState = {
  colors: {},
  hidden: {},
  academicYear: config.academicYear,
+  titleDisplay: HIDE,


Any reason why this isn't a boolean? It seems like in the end it's assigned to a boolean prop in Timetable, and the toggling and checking code will be simpler.

Also, this is the wrong place for this data. Try putting it in theme or settings state instead

@taneliang I was referring to the orientation toggling code when I wrote this, so I thought it had to have it's own type. I've simplified it to a boolean.

@ZhangYiJiang Moved the data from timetables to theme

ZhangYiJiang

Good job! Please update the test snapshots the export reducer (move the data to under settings or theme reducer first)

ZhangYiJiang · 2018-01-13T03:05:19Z

www/src/js/reducers/timetables.js

@@ -125,10 +127,11 @@ export const defaultTimetableState: TimetablesState = {
  colors: {},
  hidden: {},
  academicYear: config.academicYear,
+  titleDisplay: HIDE,


Also, this is the wrong place for this data. Try putting it in theme or settings state instead

ZhangYiJiang · 2018-01-13T03:07:50Z

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

+          type="button"
+          className="btn btn-outline-primary btn-svg"
+          onClick={props.toggleTitleDisplay}
+          disabled={isVerticalOrientation}


Better to switch to outline-secondary or hide the button entirely when its disabled. Bootstrap's disabled style for outline buttons is not very noticeable

ZhangYiJiang · 2018-01-13T03:09:11Z

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

  onModifyCell?: Function,
 };

 function TimetableCell(props: Props) {
  const lesson = props.lesson;
+  const moduleCodeWithTitle = `${lesson.ModuleCode} ${lesson.ModuleTitle}`;


Should just inline this - no need for another variable

I agree, I wrote this because the linter was going crazy, and I didn't know how to format the ternary operator properly to avoid one long line. Anyway, I figured it out and fixed it.

ZhangYiJiang · 2018-01-13T03:09:21Z

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

@@ -39,7 +43,7 @@ function TimetableCell(props: Props) {
      style={props.style}
    >
      <div className={styles.cellContainer}>
-        <div className={styles.moduleCode}>{lesson.ModuleCode}</div>
+        <div className={styles.moduleCode}>{moduleName}</div>


Also update this class name

ZhangYiJiang · 2018-01-13T03:18:31Z

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

+          onClick={props.toggleTitleDisplay}
+          disabled={isVerticalOrientation}
+        >
+          <Sidebar className={styles.sidebarIcon} />


This icon isn't appropriate. We may be able to get away with abusing the icon for credit cards, or find another one in Font Awesome or Icons8

How about the type icon?

I think the eye icon would fit in well. I've changed it to show the eye icon when titles are hidden, and the eye-off icon when titles are shown.

We're already using the eye icon to hide modules though, and our horizontal/vertical mode icon is too similar to the credit card icon IMO. I think the type icon is probably the best choice here

Ok, I've changed it to be the type icon.

taneliang · 2018-01-13T15:05:33Z

www/src/styles/utils/themes.scss

@@ -9,11 +9,11 @@
    $index: 0;

    @each $color in $colors {
+
+      $index: $index + 1; /* stylelint-disable-line order/order */


All the theme colours have been shifted by one and are therefore missing their first color. I may be wrong but I think it's because this line was moved

I don't remember changing that file, but it seems to be caused by incrementing $index before applying the colour at index 0. Fixed it.

ZhangYiJiang · 2018-01-13T15:23:47Z

It's a StyleLint bug. Lines that are marked as ignore are only ignored for linting, not autofix.

taneliang · 2018-01-13T15:40:03Z

2 UI issues:

On portrait iPad, the first row buttons get squashed. The new button should probably have its own row, but I'm not sure if we should allocate that much space for a disabled button.
Looks like the Type icon needs to be shrunk a little as it's making the buttons a bit taller.

taneliang

LGTM 👍 Can also confirm that export works.

- Update test (switch to non-default state for showTitle) - Fix showTitle button styles and classnames

shanwpf added 3 commits January 13, 2018 05:22

Add option to display module titles in timetable cells

bf1e1b6

Merge remote-tracking branch 'upstream/master'

a5579d0

Resolve incompatible type error

57692ee

taneliang reviewed Jan 13, 2018

View reviewed changes

ZhangYiJiang requested changes Jan 13, 2018

View reviewed changes

shanwpf added 5 commits January 13, 2018 15:58

Move data to theme and update tests

0df8055

Merge branch 'master' into master

c06e6bc

Use a more suitable icon for title button

9695f64

Use btn-outline-secondary when title button is disabled

a782ce0

Use text icon for title button

0615de0

taneliang reviewed Jan 13, 2018

View reviewed changes

Fix first theme colour not displaying on timetable

ffbe264

shanwpf added 2 commits January 14, 2018 00:30

Hide title button in vertical mode

b836160

Fix warnings

f1308c0

taneliang approved these changes Jan 14, 2018

View reviewed changes

Minor fixes

bda98cc

- Update test (switch to non-default state for showTitle) - Fix showTitle button styles and classnames

ZhangYiJiang approved these changes Jan 14, 2018

View reviewed changes

taneliang merged commit 2bd8d93 into nusmodifications:master Jan 14, 2018

taneliang mentioned this pull request Jan 14, 2018

Fix iPhone 5 and iOS 10 /timetable UI issues #710

Merged

ZhangYiJiang mentioned this pull request Jan 15, 2018

Display module title in timetable cell #677

Closed

Display module title in timetable cell #709

Display module title in timetable cell #709

Uh oh!

Conversation

shanwpf commented Jan 12, 2018

Screenshots

Horizontal mode single row

Horizontal mode 2 rows

Vertical mode

Uh oh!

mods-bot commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taneliang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangYiJiang Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangYiJiang left a comment

Choose a reason for hiding this comment

Uh oh!

ZhangYiJiang Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taneliang Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shanwpf Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangYiJiang commented Jan 13, 2018

Uh oh!

taneliang commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taneliang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mods-bot commented Jan 12, 2018 •

edited

Loading

ZhangYiJiang Jan 13, 2018 •

edited

Loading

ZhangYiJiang Jan 13, 2018 •

edited

Loading

taneliang Jan 13, 2018 •

edited

Loading

shanwpf Jan 13, 2018 •

edited

Loading

taneliang commented Jan 13, 2018 •

edited

Loading

taneliang left a comment •

edited

Loading