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

[#11254] Instructor Feedback Session Results: Session Duration formatting #11272

Conversation

ericluoliu
Copy link
Contributor

Fixes #11254

Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

Hi @ericluoliu, it looks like the issue still persists even with these changes! For example:

Screenshot 2021-07-17 at 11 50 06 PM

With your suggested change along with the existing line of code below, the entire session duration text will consist of non-breaking spaces, but because it's too long, it still has to break somewhere (leading to the situation in the image above):

<p class="form-control-static">{{ formattedSessionOpeningTime }}&nbsp;&nbsp;&nbsp;<b>to</b>&nbsp;&nbsp;&nbsp;{{ formattedSessionClosingTime }}</p>

To resolve this, we could perhaps either: use all normal spaces everywhere, but make a line-break after the 'to'; or we could use normal spaces before and after the 'to', along with non-breaking spaces in the formatted opening/closing times. (I might favour the first approach actually!)

@halfwhole halfwhole added the s.Ongoing The PR is being worked on by the author(s) label Jul 17, 2021
@ericluoliu
Copy link
Contributor Author

Hi @halfwhole , sorry for the very delayed response. I made the suggested changes and reverted my old changes.

Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

The session duration box now looks like this, as the whitespaces before the 'to' are collapsed into a single space, and the whitespaces after the <br> are ignored:

Screenshot 2021-08-03 at 1 59 57 PM

May I suggest changing the 3 whitespaces before the 'to' back to non-breaking spaces, and simply removing the 3 whitespaces after the linebreak? That should resolve the issue

@ericluoliu
Copy link
Contributor Author

Done.

Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

LGTM

@halfwhole halfwhole added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Aug 3, 2021
@ericluoliu
Copy link
Contributor Author

image

E2E Tests give me an error. I think it is because it expects just a tab rather than an entire line break in the spot we changed.

Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

In that case, could you update the relevant code used in the E2E tests too? The relevant lines should be here:

private String getSessionDurationString(FeedbackSessionAttributes feedbackSession) {
return getDateString(feedbackSession.getStartTime(), feedbackSession.getTimeZone()) + " to "
+ getDateString(feedbackSession.getEndTime(), feedbackSession.getTimeZone());
}

@halfwhole halfwhole added s.Ongoing The PR is being worked on by the author(s) and removed s.FinalReview The PR is ready for final review labels Aug 4, 2021
@halfwhole halfwhole added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Aug 5, 2021
@madanalogy madanalogy requested review from ccyccyccy and rrtheonlyone and removed request for rrtheonlyone and ccyccyccy August 13, 2021 08:57
@nusoss-bot
Copy link

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

Copy link
Contributor

@rrtheonlyone rrtheonlyone left a comment

Choose a reason for hiding this comment

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

LGTM

@rrtheonlyone rrtheonlyone 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 21, 2021
@rrtheonlyone rrtheonlyone merged commit 4167a05 into TEAMMATES:master Aug 21, 2021
@wkurniawan07 wkurniawan07 added the c.Bug Bug/defect report label Aug 22, 2021
@wkurniawan07 wkurniawan07 added this to the V8.1.0 milestone Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructor Feedback Session Results: Session Duration formatting
6 participants