[#4552] Increase course ID, course name, and session name max length #11277

samuelfangjw · 2021-07-16T16:20:40Z

Outline of Solution

Increased max length as follows:

course id: 40 -> 64
course name: 64 -> 80
session name: 38 -> 64

Made changes to frontend to ensure that text does not overflow their containers. This was done mainly by allowing text to wrap if necessary and increasing the size of text containers. Screenshots of some of the UI below. Two test cases were used, names with words "very long..." and names with letters only "MMMMM", M chosen because it is the widest possible letter. This represents a typical case and the extreme case where the maximum possible length is reached. Both cases have the maximum number of characters. Screenshots below were taken on a 13 inch screen. There is slightly less wrapping on larger screens.

In the screenshot above, I was a little unsure where to put the timezone field that was originally on the right side beside the course id. I've placed it below the course id as this is the the current location when the window size is too small.

For the course name, in the common case the text fits over two lines. However in the extreme case the text may take up 3 lines.

For edit course details section, the course name overflows the input in the extreme case. This is similar to current behaviour when screen width is too small.

Notes
Couldn't seem to get text-break css class to work on <td> tags. I added a td-text-break class with the same css properties to workaround this issue.

damithc · 2021-07-17T03:48:22Z

Nice work @samuelfangjw I have no issues with the UI changes you have posted.

ccyccyccy · 2021-07-19T06:20:15Z

src/web/app/pages-student/student-home-page/student-home-page.component.scss

@@ -1,3 +1,8 @@
 .margin-0 {
  margin: 0;
 }
+
+.td-text-break {


Could we use bootstrap's text-break instead?

There's a bug with text-break in the current version of bootstrap css file used (4.5.0) that causes it to not work with certain tags, <td> being one of them.

v 4.5.0

v 4.4.1

I used the td-text-break class to work around this. If this is not suitable maybe I could wrap all the necessary tags with a span and apply the text-break class to the span instead.

edit: Actually would it be possible to look into changing the version of bootstrap stylesheet used?

I think the approach of using td-text-break is fine. Just put it in the global styles.scss to avoid repeating it everywhere, and leave a comment there to explain the rationale for this class.

Actually would it be possible to look into changing the version of bootstrap stylesheet used?

What version do you want to change to? Is this issue fixed in a later version of bootstrap? We could create a separate issue for it since there may be breaking changes in changing the version. For this PR, I think we best stick to the current bootstrap version.

Changing it is possible but unlikely within the context of one PR, but if many bugs require a change or we do a major patch we can make the change.

For now, what about moving .td-text-break to a common scss file (such as page.component). This way we can also centralize the class name so we can also file an issue/ add to wiki to remove it when we update bootstrap.

src/web/app/pages-session/session-submission-page/session-submission-page.component.scss

ChooJeremy · 2021-07-20T15:07:35Z

src/web/app/pages-student/student-home-page/student-home-page.component.scss

@@ -1,3 +1,8 @@
 .margin-0 {
  margin: 0;
 }
+
+.td-text-break {


Changing it is possible but unlikely within the context of one PR, but if many bugs require a change or we do a major patch we can make the change.

For now, what about moving .td-text-break to a common scss file (such as page.component). This way we can also centralize the class name so we can also file an issue/ add to wiki to remove it when we update bootstrap.

samuelfangjw · 2021-07-21T08:41:16Z

@ccyccyccy @ChooJeremy Thanks for the review, I've taken your suggestions and moved the class to the global styles.scss file. I've renamed it text-break-class as I thought it would be neater if I was able to use this on all the tags that require it and not just td tags. I've also added a comment about the bug with link to the issue on bootstrap.

ChooJeremy

I did some testing on my end and it seems like the courses page hasn't been handled.

ChooJeremy · 2021-07-22T16:49:44Z

src/web/styles.scss

+// text-break class on current version of bootstrap (4.5.0) does not work on some tags.
+// This class can be replaced with text-break once bootstrap version is upgraded.
+// See https://github.com/twbs/bootstrap/issues/30803
+.text-break-class {


The word class here seems useless, since this is marked as a css class with the dot in front of it. What do you think about just text-break?

My original intention here was to choose a different name from text-break as other classes may use the current broken implementation of text-break class and I was a bit afraid that changing the behaviour of text-break might lead to regressions.

However, after giving this some thought, maybe it might be better to override the text-break class with the correct implementation so that future contributors who use the text-break class get the expected implementation. I could also check the all current uses of text-break to ensure that there are no regressions.

I'll take a closer look at this and fix the issues above tonight!

I renamed text-break-class to text-break and replaced the implementation with the current bootstrap implementation of text-break. I think this will make it easier to remove in the future if we ever update bootstrap as well (can just remove the style in css file).

I couldn't seem to replicate the issue on the courses page, the text breaks nicely for me. I've updated the implementation though, so hopefully this fixes it!

ChooJeremy

Found further issues:
Within the admin search page, searching for the course:

On trying to create a new feedback session, copying from previous feedback session.

On trying to edit a feedback session, copying a question from another feedback question:

There may be more. To be comprehensive, other than just manually checking pages, you can consider searching for (ctrl-shift-F in IntelliJ) cases of courseId, courseName and feedbackSessionName, with a .html filter.

ChooJeremy · 2021-07-25T16:08:57Z

src/web/styles.scss

@@ -283,3 +283,11 @@ h6,
  background: white;
  color: black;
 }
+
+// text-break class on current version of bootstrap (4.5.0) does not work on some tags.
+// This class can be replaced with text-break once bootstrap version is upgraded.


Your solution seems to work. Do remember to update the comment as well.

# Conflicts: # src/web/app/pages-instructor/instructor-audit-logs-page/__snapshots__/instructor-audit-logs-page.component.spec.ts.snap # src/web/app/pages-instructor/instructor-audit-logs-page/instructor-audit-logs-page.component.html

samuelfangjw · 2021-07-28T10:30:12Z

So sorry, i've taken a more thorough look and believe I have fixed all the text overflow issues.

ChooJeremy

LGTM; thanks for the help in searching through which pages to update and ensuring they all support this feature!

rrtheonlyone

LGTM 💯 Thanks for the work @samuelfangjw and thorough review @ChooJeremy

wkurniawan07 · 2021-08-07T04:42:20Z

@samuelfangjw can you resolve the new test failures?

M chosen because it is the widest possible letter.

It's W, actually, although the difference is rather minor 😄

samuelfangjw · 2021-08-07T05:49:41Z

@samuelfangjw can you resolve the new test failures?

M chosen because it is the widest possible letter.

It's W, actually, although the difference is rather minor 😄

@wkurniawan07 Test failures have been resolved!

samuelfangjw added 7 commits July 14, 2021 00:04

Increase session name max length to 64

ac77768

Increase course name and id max length

2d61450

Update test snapshots

cb52896

Update component tests

d5dce35

Fix buttons on student home page overflowing

821bdf9

Fix text overflow issues

475da60

Fix text overflow issues

ddc87c5

samuelfangjw marked this pull request as draft July 16, 2021 16:20

Merge branch 'master' into 4552-course-name-id-max-length

112c7e4

samuelfangjw marked this pull request as ready for review July 17, 2021 06:28

halfwhole added the s.ToReview The PR is waiting for review(s) label Jul 17, 2021

rrtheonlyone requested review from ccyccyccy and ChooJeremy July 18, 2021 08:16

ccyccyccy reviewed Jul 19, 2021

View reviewed changes

ChooJeremy reviewed Jul 20, 2021

View reviewed changes

samuelfangjw added 4 commits July 21, 2021 15:48

Merge branch 'master' into 4552-course-name-id-max-length

b1a0a09

Move text-break class to global css file

91edcf3

Remove unused css properties

6362918

Revert accidental changes to styles.scss

de1836a

Update failing snapshots

12e51fc

ChooJeremy requested changes Jul 22, 2021

View reviewed changes

samuelfangjw added 3 commits July 24, 2021 00:15

Merge branch 'master' into 4552-course-name-id-max-length

d4501ff

Rename text-break-class to text-break

85a3ea6

Update text-break style

c3ab563

samuelfangjw requested a review from ChooJeremy July 23, 2021 17:21

ChooJeremy requested changes Jul 25, 2021

View reviewed changes

samuelfangjw marked this pull request as draft July 26, 2021 08:20

samuelfangjw added 2 commits July 28, 2021 14:48

Update comment for text-break class

71c1044

Fix more text-overflow issues

4041ba5

samuelfangjw marked this pull request as ready for review July 28, 2021 10:29

samuelfangjw requested a review from ChooJeremy July 28, 2021 10:29

ChooJeremy approved these changes Jul 29, 2021

View reviewed changes

ChooJeremy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jul 29, 2021

ChooJeremy requested a review from ccyccyccy July 29, 2021 14:07

rrtheonlyone approved these changes Aug 1, 2021

View reviewed changes

rrtheonlyone and others added 2 commits August 1, 2021 11:52

Merge branch 'master' into 4552-course-name-id-max-length

70e5e40

Merge branch 'master' into 4552-course-name-id-max-length

b8fe695

ccyccyccy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Aug 6, 2021

samuelfangjw added 2 commits August 7, 2021 12:46

Merge branch 'master' into 4552-course-name-id-max-length

0deff35

Update test snapshots

468d9de

wkurniawan07 added this to the V8.0.0 milestone Aug 7, 2021

wkurniawan07 added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Aug 7, 2021

samuelfangjw and others added 2 commits August 7, 2021 13:12

Merge branch 'master' into 4552-course-name-id-max-length

3cd83b2

Merge branch 'master' into 4552-course-name-id-max-length

51fc3a7

wkurniawan07 merged commit eb550ce into TEAMMATES:master Aug 7, 2021

wkurniawan07 assigned ChooJeremy, rrtheonlyone and ccyccyccy Aug 7, 2021

samuelfangjw deleted the 4552-course-name-id-max-length branch August 17, 2021 02:11

samuelfangjw mentioned this pull request Mar 6, 2022

Remove overridden text-break class #11633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4552] Increase course ID, course name, and session name max length #11277

[#4552] Increase course ID, course name, and session name max length #11277

samuelfangjw commented Jul 16, 2021

damithc commented Jul 17, 2021

ccyccyccy Jul 19, 2021

samuelfangjw Jul 19, 2021 •

edited

ccyccyccy Jul 20, 2021

ChooJeremy Jul 20, 2021

ChooJeremy Jul 20, 2021

samuelfangjw commented Jul 21, 2021

ChooJeremy left a comment

ChooJeremy Jul 22, 2021

samuelfangjw Jul 23, 2021

samuelfangjw Jul 23, 2021 •

edited

ChooJeremy left a comment

ChooJeremy Jul 25, 2021

samuelfangjw commented Jul 28, 2021

ChooJeremy left a comment

rrtheonlyone left a comment

wkurniawan07 commented Aug 7, 2021

samuelfangjw commented Aug 7, 2021

[#4552] Increase course ID, course name, and session name max length #11277

[#4552] Increase course ID, course name, and session name max length #11277

Conversation

samuelfangjw commented Jul 16, 2021

damithc commented Jul 17, 2021

ccyccyccy Jul 19, 2021

Choose a reason for hiding this comment

samuelfangjw Jul 19, 2021 • edited

Choose a reason for hiding this comment

ccyccyccy Jul 20, 2021

Choose a reason for hiding this comment

ChooJeremy Jul 20, 2021

Choose a reason for hiding this comment

ChooJeremy Jul 20, 2021

Choose a reason for hiding this comment

samuelfangjw commented Jul 21, 2021

ChooJeremy left a comment

Choose a reason for hiding this comment

ChooJeremy Jul 22, 2021

Choose a reason for hiding this comment

samuelfangjw Jul 23, 2021

Choose a reason for hiding this comment

samuelfangjw Jul 23, 2021 • edited

Choose a reason for hiding this comment

ChooJeremy left a comment

Choose a reason for hiding this comment

ChooJeremy Jul 25, 2021

Choose a reason for hiding this comment

samuelfangjw commented Jul 28, 2021

ChooJeremy left a comment

Choose a reason for hiding this comment

rrtheonlyone left a comment

Choose a reason for hiding this comment

wkurniawan07 commented Aug 7, 2021

samuelfangjw commented Aug 7, 2021

samuelfangjw Jul 19, 2021 •

edited

samuelfangjw Jul 23, 2021 •

edited