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

[#12116] Instructor viewing results of rubric questions: missing space after checkbox #12151

Merged

Conversation

Trumerik
Copy link
Contributor

@Trumerik Trumerik commented Mar 1, 2023

Fixes #12116

Outline of Solution
Added a space in the beginning of the Exclude self evaluation text in rubric-question-statistics.component.html
image

@weiquu
Copy link
Contributor

weiquu commented Mar 2, 2023

Hi @Trumerik, thanks for contributing (: The fix looks good, but I'm wondering if we can make it better by adding the space between the checkbox and the text via css instead. This removes the additional dotted line between the checkbox and the text:
Screenshot 2023-03-02 at 8 45 01 PM

Small change, but I think it looks a bit better! Am open to hearing if you have any other suggestions or views as well

@Trumerik
Copy link
Contributor Author

Trumerik commented Mar 2, 2023

Hi @weiquu, I totally agree with you! I have now pushed a new solution making use of margin-left to get the space without the additional dotted line! Below is a picture of how it looks now:
image

@damithc
Copy link
Contributor

damithc commented Mar 3, 2023

@weiquu I assume we have similar 'checkbox followed by text' in other places of the UI. What technique are we using in those places to create space between the checkbox and the text? It is better if we use the same technique everywhere, for consistency.
If the problem occurs in other places too, perhaps we can do the tweak in a way that it is implemented in one place and applied everywhere applicable?

@weiquu
Copy link
Contributor

weiquu commented Mar 3, 2023

Thanks @damithc, will take a look later tonight. @Trumerik feel free to help check as well (:

@@ -8,7 +8,7 @@
</div>
<div class="col-sm-8" style="text-align: right;">
<label class="form-check-label">
<input id="exclude-self-checkbox" type="checkbox" [(ngModel)]="excludeSelf" (ngModelChange)="getTableData()" class="form-check-input"> <span container="body" class="ngb-tooltip-class" ngbTooltip="Excludes giver's responses to himself/herself from statistics">Exclude self evaluation</span>
<input id="exclude-self-checkbox" type="checkbox" [(ngModel)]="excludeSelf" (ngModelChange)="getTableData()" class="form-check-input"> <span container="body" class="ngb-tooltip-class" ngbTooltip="Excludes giver's responses to himself/herself from statistics" style="margin-left: .33rem;">Exclude self evaluation</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick note: let's try to use bootstrap styles as much as possible. But let's see if there are any other checkbox spaces issues first!

@Trumerik
Copy link
Contributor Author

Trumerik commented Mar 3, 2023

I tried looking into some similar 'checkbox followed by text' in other places and found these:

image
For all the above it seems a simple space has been added at the beginning of the text as I did in my first push, but as there is no dotted line here it works. Below is an example for "Group by Teams" taken from the code:
<label class="form-check-label" ><input id="include-team-grouping" type="checkbox" [ngModel]="groupByTeam" (ngModelChange)="handleGroupByTeamChange($event)" [disabled]="viewType === 'QUESTION'"> Group by Teams</label>

image
For this it seems that &nbsp;, which is an HTML entity that represents a non-breaking space, has been added before the span. here is the code:
&nbsp;
<span class="ngb-tooltip-class" ngbTooltip="If this is unselected, the instructor will be completely invisible to students. E.g. to give access to a colleague for ‘auditing’ your course">Display to students as:</span>

@weiquu
Copy link
Contributor

weiquu commented Mar 4, 2023

Thanks @Trumerik for the research! I found a few where they used &nbsp; as you mentioned, but I think the majority used the form-check class on the surrounding div (e.g. contribution-question-edit-details-form.component.html). This was the case both for text with and without the dotted line.

According to the Bootstrap docs, .form-check automatically improves the layout and helps with consistency, so I think using that would be a better option moving forward. Sorry for the many changes!

@Trumerik
Copy link
Contributor Author

Trumerik commented Mar 4, 2023

Thank you @weiquu!
I have now pushed a form-check solution.
image

Comment on lines 9 to 10
<div class="col-sm-8" style="text-align: right;">
<label class="form-check-label">
<input id="exclude-self-checkbox" type="checkbox" [(ngModel)]="excludeSelf" (ngModelChange)="getTableData()" class="form-check-input"> <span container="body" class="ngb-tooltip-class" ngbTooltip="Excludes giver's responses to himself/herself from statistics">Exclude self evaluation</span>
</label>
<div class="form-check">
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the .form-check class can be placed directly in the above div (i.e. the div with class col-sm-8). Then we don't need to add in the extra div

@weiquu
Copy link
Contributor

weiquu commented Mar 5, 2023

Left one small comment, other than that looks good (: Pinging @zhaojj2209 / @samuelfangjw

@Trumerik
Copy link
Contributor Author

Trumerik commented Mar 5, 2023

You are correct, thank you @weiquu
I pushed the correction.

Copy link
Member

@samuelfangjw samuelfangjw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing to TEAMMATES!

@samuelfangjw samuelfangjw added the s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging label Mar 5, 2023
@samuelfangjw samuelfangjw added this to the V8.24.0 milestone Mar 5, 2023
@zhaojj2209 zhaojj2209 merged commit 3458565 into TEAMMATES:master Mar 5, 2023
@samuelfangjw samuelfangjw added the c.Bug Bug/defect report label Mar 9, 2023
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 viewing results of rubric questions: missing space after checkbox
5 participants