Fix part of oppia#18384: Added RTL support for learner dashboard rede…

…sign (oppia#20744) * Removed lastCard var from cards * Fixed RTL spacing and modified carousel for RTL * Renamed function to more generic names * Test cases for RTL, reworked test case descriptions to be clearer * Fixed test cases * Removed unused var --------- Co-authored-by: Akhilesh Kr. <[email protected]>
AFZL210 · Aug 9, 2024 · 2932d9c · 2932d9c
1 parent a32bfc3
commit 2932d9c
Show file tree

Hide file tree

Showing 11 changed files with 205 additions and 94 deletions.
diff --git a/core/templates/components/summary-tile/learner-topic-summary-tile.component.html b/core/templates/components/summary-tile/learner-topic-summary-tile.component.html
@@ -23,7 +23,7 @@
       </div>
     </a>
   </mat-card>
-  <a *ngIf="redesignFeatureFlag" class="d-flex flex-column flex-shrink-0 oppia-class-card text-decoration-none" [href]="getTopicLink()" target="{{ openInNewWindow ? '_blank' : '_self' }}" [ngClass]="{'oppia-class-card-not-last': !lastCard}">
+  <a *ngIf="redesignFeatureFlag" class="d-flex flex-column flex-shrink-0 oppia-class-card text-decoration-none" [href]="getTopicLink()" target="{{ openInNewWindow ? '_blank' : '_self' }}">
     <div class="oppia-class-card-img rounded-top" [ngStyle]="{'background-color': thumbnailBgColor}">
       <img alt="" class="oppia-thumbnail-image" [src]="thumbnailUrl">
     </div>
@@ -102,10 +102,6 @@
     box-shadow: 0 0 8px 0 #00000040;
   }
 
-  .oppia-class-card-not-last {
-    margin-right: 16px;
-  }
-
   .oppia-class-card-button {
     border: 1px solid #00645c;
     font-size: 14px;

diff --git a/core/templates/components/summary-tile/learner-topic-summary-tile.component.ts b/core/templates/components/summary-tile/learner-topic-summary-tile.component.ts
@@ -41,7 +41,6 @@ export class LearnerTopicSummaryTileComponent implements OnInit {
   thumbnailBgColor!: string;
   openInNewWindow = false;
   @Input() redesignFeatureFlag!: boolean;
-  @Input() lastCard!: boolean;
 
   constructor(
     private urlInterpolationService: UrlInterpolationService,

diff --git a/core/templates/components/summary-tile/lesson-card.component.html b/core/templates/components/summary-tile/lesson-card.component.html
@@ -1,5 +1,4 @@
-<!--TODO(#18384): Fix when there's only one card, margin-right still appears-->
-<div class="lesson-card-container" [ngClass]="{'lesson-card-not-last': !lastCard}">
+<div class="lesson-card-container">
   <div class="align-items-center d-flex justify-content-center lesson-card-img relative rounded-top w-100" [ngStyle]="{'background-color': imgColor}">
     <img alt="" class="h-100" [src]="imgUrl">
     <div class="d-flex flex-column lesson-card-tags w-100">
@@ -87,8 +86,4 @@
     font-size: 11px;
     -webkit-line-clamp: 2;
   }
-
-  .lesson-card-not-last {
-    margin-right: 16px;
-  }
 </style>
diff --git a/core/templates/components/summary-tile/lesson-card.component.ts b/core/templates/components/summary-tile/lesson-card.component.ts
@@ -34,7 +34,6 @@ import {StoryNode} from 'domain/story/story-node.model';
 export class LessonCardComponent implements OnInit {
   @Input() story!: StorySummary | LearnerExplorationSummary | CollectionSummary;
   @Input() topic!: string;
-  @Input() lastCard!: boolean;
   @Input() isCommunityLessonComplete?: boolean;
 
   desc!: string;

diff --git a/core/templates/pages/learner-dashboard-page/card-display/card-display.component.html b/core/templates/pages/learner-dashboard-page/card-display/card-display.component.html
@@ -1,13 +1,12 @@
 <div class="d-flex flex-column h-100 mb-4 w-100" [ngStyle]="{'max-width': (tabType === 'homeFull' || tabType.includes('progress') ? '100%' : 'fit-content')}">
   <div class="d-flex justify-content-between">
     <p class="card-display-heading"> {{ headingI18n | translate }} </p>
-    <div class="d-flex" *ngIf="(numCards > 1) && ((numCards - 1) * cardWidth + (cardWidth - 32) > cards.offsetWidth) && (tabType.includes('home'))">
-      <button [disabled]="currentShift === 0" class="mr-2 px-0 card-display-button" (click)="nextCard(currentShift - 1)">
-        <span class="fas fa-chevron-left"></span>
+    <div class="card-display-button-container" *ngIf="(numCards > 1) && ((numCards - 1) * cardWidth + (cardWidth - 32) > cards.offsetWidth) && (tabType.includes('home'))">
+      <button [disabled]="currentShift === 0" class="px-0 card-display-button" (click)="moveCard(currentShift - 1)">
+        <span [ngClass]="'fas fa-chevron-' + (isLanguageRTL ? 'right' : 'left')"></span>
       </button>
-      <p> </p>
-      <button [disabled]="currentShift === getMaxShifts(cards.offsetWidth)" class="px-0 card-display-button" (click)="nextCard(currentShift + 1)">
-        <span class="fas fa-chevron-right"></span>
+      <button [disabled]="currentShift === getMaxShifts(cards.offsetWidth)" class="px-0 card-display-button" (click)="moveCard(currentShift + 1)">
+        <span [ngClass]="'fas fa-chevron-' + (isLanguageRTL ? 'left' : 'right')"></span>
       </button>
     </div>
   </div>
@@ -63,6 +62,7 @@
 
   .card-display-content-container {
     display: flex;
+    gap: 0 16px;
     max-width: 100%;
     overflow-x: hidden;
     padding: .5px;
@@ -74,4 +74,9 @@
     font-weight: 500;
     margin-bottom: 0;
   }
+
+  .card-display-button-container {
+    display: flex;
+    gap: 0 5px;
+  }
 </style>
diff --git a/core/templates/pages/learner-dashboard-page/card-display/card-display.component.spec.ts b/core/templates/pages/learner-dashboard-page/card-display/card-display.component.spec.ts
@@ -43,13 +43,15 @@ describe('CardDisplayComponent', () => {
     component.numCards = 5;
     component.tabType = 'home';
     component.headingI18n = 'I18N_LEARNER_DASHBOARD_HOME_SAVED_SECTION';
+    component.isLanguageRTL = false;
     fixture.detectChanges();
 
     spyOnProperty(
       component.cards.nativeElement,
       'offsetWidth',
       'get'
     ).and.returnValue(400);
+
     scrollLeftSetterSpy = spyOnProperty(
       component.cards.nativeElement,
       'scrollLeft',
@@ -67,90 +69,191 @@ describe('CardDisplayComponent', () => {
     expect(component.getMaxShifts(400)).toEqual(4);
   });
 
-  describe('when shifting cards container to the right', () => {
-    it('should shift by (cardWidth - 32) if first shift to the right', () => {
-      component.nextCard(1);
-
+  describe('LTR: ', () => {
+    beforeEach(() => {
+      component.isLanguageRTL = false;
       fixture.detectChanges();
-
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(200);
     });
 
-    it('should shift by cardWidth if not first shift or last to the right', () => {
-      spyOnProperty(
-        component.cards.nativeElement,
-        'scrollLeft',
-        'get'
-      ).and.returnValue(200);
+    describe('when shifting to next card', () => {
+      it('should add (cardWidth - 32) if first shift', () => {
+        component.moveCard(1);
 
-      component.currentShift = 1;
-      component.nextCard(2);
+        fixture.detectChanges();
 
-      fixture.detectChanges();
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(200);
+      });
+
+      it('should add cardWidth if not first or last shift', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(200);
+
+        component.currentShift = 1;
+        component.moveCard(2);
+
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(432);
+      });
+
+      it('should add remainder if last shift, setting scrollLeft = div width', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(664);
+
+        component.currentShift = 3;
+        component.moveCard(4);
 
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(432);
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(756.5);
+      });
     });
 
-    it('should shift by remainder needed to show last card if last shift to the right', () => {
-      spyOnProperty(
-        component.cards.nativeElement,
-        'scrollLeft',
-        'get'
-      ).and.returnValue(664);
+    describe('when shifting to previous card', () => {
+      it('should subtract remainder if from last shift', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(756.5);
 
-      component.currentShift = 3;
-      component.nextCard(4);
+        component.currentShift = 4;
+        component.moveCard(3);
 
-      fixture.detectChanges();
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(664);
+      });
+
+      it('should subtract cardWidth if not first or last shift', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(664);
+
+        component.currentShift = 3;
+        component.moveCard(2);
+
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(432);
+      });
+
+      it('should subtract (cardWidth - 32) to reset scrollLeft = 0', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(200);
+
+        component.currentShift = 1;
+        component.moveCard(0);
 
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(756.5);
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(0);
+      });
     });
   });
 
-  describe('when shifting cards container to the left', () => {
-    it('should shift by remainder needed to show last if first shift', () => {
-      spyOnProperty(
-        component.cards.nativeElement,
-        'scrollLeft',
-        'get'
-      ).and.returnValue(756.5);
+  describe('RTL: ', () => {
+    beforeEach(() => {
+      component.isLanguageRTL = true;
+      fixture.detectChanges();
+    });
 
-      component.currentShift = 4;
-      component.nextCard(3);
+    describe('when shifting to next card', () => {
+      it('should subtract (cardWidth - 32) if first shift', () => {
+        component.moveCard(1);
 
-      fixture.detectChanges();
+        fixture.detectChanges();
 
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(664);
-    });
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(-200);
+      });
 
-    it('should shift by cardWidth if not first shift or last', () => {
-      spyOnProperty(
-        component.cards.nativeElement,
-        'scrollLeft',
-        'get'
-      ).and.returnValue(664);
+      it('should subtract by cardWidth if not first or last shift', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(-200);
 
-      component.currentShift = 3;
-      component.nextCard(2);
+        component.currentShift = 1;
+        component.moveCard(2);
 
-      fixture.detectChanges();
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(-432);
+      });
+
+      it('should subtract remainder if last shift, setting scrollLeft = -div width', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(-664);
+
+        component.currentShift = 3;
+        component.moveCard(4);
+
+        fixture.detectChanges();
 
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(432);
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(-756.5);
+      });
     });
 
-    it('should shift by (cardWidth - 32) if last shift', () => {
-      spyOnProperty(
-        component.cards.nativeElement,
-        'scrollLeft',
-        'get'
-      ).and.returnValue(200);
+    describe('when shifting to previous card', () => {
+      it('should add remainder if from last card', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(-756.5);
 
-      component.currentShift = 1;
-      component.nextCard(0);
+        component.currentShift = 4;
+        component.moveCard(3);
 
-      fixture.detectChanges();
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(-664);
+      });
+
+      it('should add cardWidth if not first or last shift', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(-664);
+
+        component.currentShift = 3;
+        component.moveCard(2);
+
+        fixture.detectChanges();
+
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(-432);
+      });
+
+      it('should add (cardWidth - 32) to reset scrollLeft = 0', () => {
+        spyOnProperty(
+          component.cards.nativeElement,
+          'scrollLeft',
+          'get'
+        ).and.returnValue(-200);
+
+        component.currentShift = 1;
+        component.moveCard(0);
+
+        fixture.detectChanges();
 
-      expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(0);
+        expect(scrollLeftSetterSpy.calls.mostRecent().args[0]).toBe(0);
+      });
     });
   });
 });