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

General: Fix scroll to section when clicking status bar in exercise update view #9243

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
296a494
Fixing scroll so headline is displayed
florian-glombik Aug 22, 2024
8e0d6ee
Removing template test
florian-glombik Aug 22, 2024
5f89b1b
Adding e2e test for scrolling
florian-glombik Aug 22, 2024
c685882
Calculating header height differently
florian-glombik Aug 22, 2024
c8767e4
Fixing scroll for safari
florian-glombik Aug 22, 2024
dfc7149
Adjusting e2e test
florian-glombik Aug 22, 2024
c77cbab
Make e2e test more reliable
florian-glombik Aug 23, 2024
58dc138
Minor refactorings
florian-glombik Aug 23, 2024
6a4c643
Fixing typo in comment
florian-glombik Aug 23, 2024
019a2f2
Fixing client test
florian-glombik Aug 23, 2024
6b57ff6
Use a smarter way than a timeout to wait for the scroll to finish
florian-glombik Aug 23, 2024
e58c8f8
Adding timeout to custom function
florian-glombik Aug 23, 2024
b320b61
Removing unintended space
florian-glombik Aug 23, 2024
212be98
Improve documentation
florian-glombik Aug 23, 2024
7315efc
Improve naming of helper methods
florian-glombik Aug 23, 2024
9d7e027
Removing magic numbers
florian-glombik Aug 23, 2024
5115456
Addressing Ramonas feedback
florian-glombik Aug 23, 2024
f1ed7a4
Improving readability of form-status-bar.component
florian-glombik Aug 23, 2024
175896e
Re-adding fix for Safari
florian-glombik Aug 23, 2024
2260168
Fixing navbar distance for Safari
florian-glombik Aug 23, 2024
26bf89b
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 23, 2024
3f3a9eb
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 25, 2024
4ecbfd6
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 26, 2024
b99ec38
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 27, 2024
4d357b0
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 27, 2024
33b5336
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 28, 2024
a1d4751
Using more robust solution to retrieve scroll offset
florian-glombik Aug 29, 2024
2b6be15
Fix broken states for formStatusSections
florian-glombik Aug 29, 2024
495a83f
Fixing client tests by not using renderer2
florian-glombik Aug 30, 2024
fc4fd47
Not counding breadcrumb container twice
florian-glombik Aug 30, 2024
d15bf08
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 30, 2024
009509d
Address Ramonas feedback
florian-glombik Aug 30, 2024
9d814b6
Fixing e2e test
florian-glombik Aug 30, 2024
83b46ce
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Aug 31, 2024
003a79f
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 1, 2024
fad95f3
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 2, 2024
6bd6400
Fixing change detection for modeling exercise
florian-glombik Sep 2, 2024
9e722a6
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 2, 2024
07b372b
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 3, 2024
dc2bc3f
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 4, 2024
d434f42
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 5, 2024
81077b7
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 5, 2024
b974a06
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 5, 2024
4ad749d
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 6, 2024
44f0f21
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 6, 2024
5689614
Merge branch 'develop' into bugfix/programming-exercises/scroll-to-he…
florian-glombik Sep 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AfterViewInit, ChangeDetectionStrategy, Component, OnDestroy, OnInit, ViewChild } from '@angular/core';
import { AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, OnDestroy, OnInit, ViewChild } from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';
import { HttpErrorResponse } from '@angular/common/http';
import { ModelingExercise } from 'app/entities/modeling-exercise.model';
Expand Down Expand Up @@ -93,6 +93,7 @@ export class ModelingExerciseUpdateComponent implements AfterViewInit, OnDestroy
private activatedRoute: ActivatedRoute,
private router: Router,
private navigationUtilService: ArtemisNavigationUtilService,
private changeDetectorRef: ChangeDetectorRef,
) {}

get editType(): EditType {
Expand Down Expand Up @@ -243,6 +244,9 @@ export class ModelingExerciseUpdateComponent implements AfterViewInit, OnDestroy
!this.modelingExercise.releaseDate?.isValid()),
},
];

// otherwise the change detection does not work on the initial load
this.changeDetectorRef.detectChanges();
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
<div class="form-status-bar-container pt-2 pb-1">
<hr class="form-status-line" />
<div class="d-flex justify-content-between">
@for (formStatusSection of formStatusSections; track formStatusSection.title) {
<div class="d-flex flex-column align-items-center">
<div
class="d-flex align-items-center justify-content-center border rounded-circle border-secondary form-status-circle"
(click)="scrollToHeadline(formStatusSection.title)"
>
<jhi-checklist-check
[checkAttribute]="formStatusSection.valid"
[iconColor]="formStatusSection.empty && formStatusSection.valid ? 'var(--bs-gray)' : undefined"
class="fa-sm"
/>
@if (formStatusSections) {
<div #statusBar class="form-status-bar-container pt-2 pb-1">
florian-glombik marked this conversation as resolved.
Show resolved Hide resolved
<hr class="form-status-line" />
<div class="d-flex justify-content-between">
@for (formStatusSection of formStatusSections; track formStatusSection.title; let index = $index) {
<div class="d-flex flex-column align-items-center" id="status-bar-section-item-{{ index }}">
<div class="form-status-circle border rounded-circle" (click)="scrollToHeadline(formStatusSection.title)">
<jhi-checklist-check
[checkAttribute]="formStatusSection.valid"
[iconColor]="formStatusSection.empty && formStatusSection.valid ? 'var(--bs-gray)' : undefined"
class="fa-sm position-absolute top-50 start-50 translate-middle"
/>
</div>
<a (click)="scrollToHeadline(formStatusSection.title)" class="d-none d-sm-inline link-primary">{{ formStatusSection.title | artemisTranslate }}</a>
</div>
<a (click)="scrollToHeadline(formStatusSection.title)" class="d-none d-sm-inline link-primary">{{ formStatusSection.title | artemisTranslate }}</a>
</div>
}
}
</div>
</div>
</div>
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
.form-status-circle {
position: relative;
z-index: 8;
height: 28px;
width: 28px;
height: 1.5rem;
width: 1.5rem;
background-color: var(--bs-card-bg);
cursor: pointer;
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { AfterViewInit, Component, HostListener, Input } from '@angular/core';
import { AfterViewInit, Component, ElementRef, HostListener, Input, ViewChild } from '@angular/core';
import { updateHeaderHeight } from 'app/shared/util/navbar.util';

export type FormSectionStatus = {
title: string;
Expand All @@ -15,23 +16,28 @@ export class FormStatusBarComponent implements AfterViewInit {
@Input()
formStatusSections: FormSectionStatus[];

@ViewChild('statusBar', { static: false }) statusBar?: ElementRef;

@HostListener('window:resize')
onResize() {
setTimeout(() => {
const headerHeight = (document.querySelector('jhi-navbar') as HTMLElement).offsetHeight;
document.documentElement.style.setProperty('--header-height', `${headerHeight}px`);
});
onResizeAddDistanceFromStatusBarToNavbar() {
updateHeaderHeight();
}

ngAfterViewInit() {
this.onResize();
this.onResizeAddDistanceFromStatusBarToNavbar();
}

scrollToHeadline(id: string) {
const element = document.getElementById(id);
if (element) {
element.style.scrollMarginTop = 'calc(2rem + 78px)';
element.scrollIntoView();
const navbarHeight = (document.querySelector('jhi-navbar') as HTMLElement)?.getBoundingClientRect().height;
const statusBarHeight = this.statusBar?.nativeElement.getBoundingClientRect().height;

/** Needs to be applied to the scrollMarginTop to ensure that the scroll to element is not hidden behind header elements */
const scrollOffsetInPx = navbarHeight + statusBarHeight;

element.style.scrollMarginTop = `${scrollOffsetInPx}px`;
element.scrollIntoView({ behavior: 'smooth', block: 'start', inline: 'start' });
}
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
@if (checkAttribute) {
<fa-icon [icon]="faCheckCircle" [size]="size" [style]="{ color: iconColor ?? 'var(--green)' }" />
} @else {
<fa-icon [icon]="faTimes" [size]="size" class="fs-5" [style]="{ color: iconColor ?? 'var(--markdown-preview-red)' }" />
}
&nbsp;
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@ import { faCheckCircle, faTimes } from '@fortawesome/free-solid-svg-icons';
@Component({
selector: 'jhi-checklist-check',
templateUrl: './checklist-check.component.html',
styleUrls: ['./checklist-check.component.scss'],
})
export class ChecklistCheckComponent {
protected readonly faTimes = faTimes;
protected readonly faCheckCircle = faCheckCircle;

@Input() checkAttribute: boolean | undefined = false;
@Input() iconColor?: string;
@Input() size?: SizeProp;

// Icons
faTimes = faTimes;
faCheckCircle = faCheckCircle;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { NgModule } from '@angular/core';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check.component';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check/checklist-check.component';
import { ArtemisSharedModule } from 'app/shared/shared.module';
import { FeatureToggleModule } from 'app/shared/feature-toggle/feature-toggle.module';
import { ConfirmAutofocusModalComponent } from 'app/shared/components/confirm-autofocus-modal.component';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { AfterViewInit, Component, HostListener, Input } from '@angular/core';
import { updateHeaderHeight } from 'app/shared/util/navbar.util';

@Component({
selector: 'jhi-detail-overview-navigation-bar',
Expand All @@ -10,15 +11,12 @@ export class DetailOverviewNavigationBarComponent implements AfterViewInit {
sectionHeadlines: { id: string; translationKey: string }[];

@HostListener('window:resize')
onResize() {
setTimeout(() => {
const headerHeight = (document.querySelector('jhi-navbar') as HTMLElement).offsetHeight;
document.documentElement.style.setProperty('--header-height', `${headerHeight - 2}px`);
});
onResizeAddDistanceFromStatusBarToNavbar() {
updateHeaderHeight();
}

ngAfterViewInit() {
this.onResize();
this.onResizeAddDistanceFromStatusBarToNavbar();
}

scrollToView(id: string) {
Expand Down
16 changes: 16 additions & 0 deletions src/main/webapp/app/shared/util/navbar.util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Update the header height SCSS variable based on the navbar height.
*
* The navbar height can change based on the screen size and the content of the navbar
* (e.g. long breadcrumbs due to longs exercise names)
*/
export function updateHeaderHeight() {
setTimeout(() => {
const navbar = document.querySelector('jhi-navbar');
if (navbar) {
// do not use navbar.offsetHeight, this might not be defined in Safari!
const headerHeight = navbar.getBoundingClientRect().height;
document.documentElement.style.setProperty('--header-height', `${headerHeight}px`);
}
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { RouterTestingModule } from '@angular/router/testing';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
import { Component } from '@angular/core';
import { HasAnyAuthorityDirective } from 'app/shared/auth/has-any-authority.directive';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check.component';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check/checklist-check.component';
import { FaIconComponent } from '@fortawesome/angular-fontawesome';
import { HttpClientTestingModule } from '@angular/common/http/testing';
import { ProgressBarComponent } from 'app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { FaIconComponent } from '@fortawesome/angular-fontawesome';
import { AccountService } from 'app/core/auth/account.service';
import { Course } from 'app/entities/course.model';
import { Exam } from 'app/entities/exam/exam.model';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check.component';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check/checklist-check.component';
import { ExamChecklistExerciseGroupTableComponent } from 'app/exam/manage/exams/exam-checklist-component/exam-checklist-exercisegroup-table/exam-checklist-exercisegroup-table.component';
import { ExamChecklistComponent } from 'app/exam/manage/exams/exam-checklist-component/exam-checklist.component';
import { ExamDetailComponent } from 'app/exam/manage/exams/exam-detail.component';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ describe('FormStatusBarComponent', () => {
jest.restoreAllMocks();
});

it('should initializes', () => {
expect(comp).toBeDefined();
});

it('should scroll to correct headline', () => {
const mockDOMElement = { scrollIntoView: jest.fn(), style: {} };
const getElementSpy = jest.spyOn(document, 'getElementById').mockReturnValue(mockDOMElement as any as HTMLElement);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { TutorialGroupsChecklistComponent } from 'app/course/tutorial-groups/tutorial-groups-management/tutorial-groups-checklist/tutorial-groups-checklist.component';
import { CourseManagementService } from 'app/course/manage/course-management.service';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check.component';
import { ChecklistCheckComponent } from 'app/shared/components/checklist-check/checklist-check.component';
import { MockRouter } from '../../../../../helpers/mocks/mock-router';
import { MockComponent, MockPipe, MockProvider } from 'ng-mocks';
import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe';
Expand Down
3 changes: 1 addition & 2 deletions src/test/playwright/e2e/course/CourseManagement.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ import { test } from '../../support/fixtures';
import dayjs from 'dayjs';
import { Course } from 'app/entities/course.model';
import { admin, studentOne } from '../../support/users';
import { convertBooleanToCheckIconClass, dayjsToString, generateUUID, trimDate } from '../../support/utils';
import { base64StringToBlob, convertBooleanToCheckIconClass, dayjsToString, generateUUID, trimDate } from '../../support/utils';
import { expect } from '@playwright/test';
import { Fixtures } from '../../fixtures/fixtures';
import { base64StringToBlob } from '../../support/utils';

// Common primitives
const courseData = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,36 @@ test.describe('Programming Exercise Management', () => {
await expect(courseManagementExercises.getExerciseTitle(exerciseTitle)).toBeVisible();
await page.waitForURL(`**/programming-exercises/${exercise.id}**`);
});

test('FormStatusBar scrolls to the correct section with headline visible after scroll', async ({
login,
page,
navigationBar,
courseManagement,
courseManagementExercises,
programmingExerciseCreation,
}) => {
await login(admin, '/');
await navigationBar.openCourseManagement();
await courseManagement.openExercisesOfCourse(course.id!);
florian-glombik marked this conversation as resolved.
Show resolved Hide resolved
await courseManagementExercises.createProgrammingExercise();
await page.waitForURL('**/programming-exercises/new**');

const firstSectionHeadline = 'General';
const firstSectionStatusBarId = 0;
const fourthSectionHeadline = 'Problem';
const fourthSectionStatusBarId = 3;

// scroll down
await programmingExerciseCreation.clickFormStatusBarSection(fourthSectionStatusBarId);
await programmingExerciseCreation.checkIsHeadlineVisibleToUser(firstSectionHeadline, false);
await programmingExerciseCreation.checkIsHeadlineVisibleToUser(fourthSectionHeadline, true);

// scroll up
await programmingExerciseCreation.clickFormStatusBarSection(firstSectionStatusBarId);
await programmingExerciseCreation.checkIsHeadlineVisibleToUser(firstSectionHeadline, true);
await programmingExerciseCreation.checkIsHeadlineVisibleToUser(fourthSectionHeadline, false);
});
});

test.describe('Programming exercise deletion', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Page, expect } from '@playwright/test';
import { Locator, Page, expect } from '@playwright/test';
import { PROGRAMMING_EXERCISE_BASE, ProgrammingLanguage } from '../../../constants';
import { Dayjs } from 'dayjs';

Expand Down Expand Up @@ -73,4 +73,62 @@ export class ProgrammingExerciseCreationPage {
}
await expect(this.page.locator('.owl-dt-popup')).not.toBeVisible();
}

/**
* Uses an element that is affected by the scrolling of the page as reference to determine if the page is still scrolling
*
* Here we are using the headline of the page as reference
*/
async waitForPageToFinishScrolling(maxTimeout: number = 5000) {
const elementOnPageAffectedByScroll = this.page.locator('h2');
let isScrolling = true;
const startTime = Date.now();

while (isScrolling) {
const initialPosition = await elementOnPageAffectedByScroll.boundingBox();
await this.page.waitForTimeout(100); // give the page a short time to scroll
const newPosition = await elementOnPageAffectedByScroll.boundingBox();

isScrolling = initialPosition?.y !== newPosition?.y;

const isWaitingForScrollExceedingTimeout = Date.now() - startTime > maxTimeout;
if (isWaitingForScrollExceedingTimeout) {
throw new Error(`Aborting waiting for scroll end - page is still scrolling after ${maxTimeout}ms`);
}
}
}

async clickFormStatusBarSection(sectionId: number) {
const searchedSectionId = `#status-bar-section-item-${sectionId}`;
const sectionStatusBarLocator: Locator = this.page.locator(searchedSectionId);
expect(await sectionStatusBarLocator.isVisible()).toBeTruthy();
await sectionStatusBarLocator.click();
await this.waitForPageToFinishScrolling();
}

/**
* Verifies that the locator is visible in the viewport and not hidden by another element
* (e.g. could be hidden by StatusBar / Navbar)
*
* {@link toBeHidden} and {@link toBeVisible} do not solve this problem
* @param locator
*/
private async verifyLocatorIsVisible(locator: Locator) {
const initialPosition = await locator.boundingBox();
await locator.click(); // scrolls to the locator if needed (e.g. if hidden by another element)
const newPosition = await locator.boundingBox();
expect(initialPosition).toEqual(newPosition);
}

async checkIsHeadlineVisibleToUser(searchedHeadlineDisplayText: string, expected: boolean) {
const headlineLocator = this.page.getByRole('heading', { name: searchedHeadlineDisplayText }).first();

if (expected) {
await expect(headlineLocator).toBeInViewport({ ratio: 1 });
/** additional check because {@link toBeInViewport} is too inaccurate */
await this.verifyLocatorIsVisible(headlineLocator);
} else {
await expect(headlineLocator).not.toBeInViewport();
}
}
}
6 changes: 4 additions & 2 deletions src/test/playwright/support/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,12 @@ export function titleLowercase(title: string) {
/**
* Converts a boolean value to its related icon class.
* @param boolean - The boolean value to be converted.
* @returns The corresponding ".checked" or ".unchecked" string.
* @returns The corresponding icon class
*/
export function convertBooleanToCheckIconClass(boolean: boolean) {
return boolean ? '.checked' : '.unchecked';
const sectionInvalidIcon = '.fa-xmark';
const sectionValidIcon = '.fa-circle-check';
return boolean ? sectionValidIcon : sectionInvalidIcon;
}

/**
Expand Down
Loading