some changes after code review

sl-design-system · Oct 23, 2024 · 74c3a86 · 74c3a86
1 parent 78cf665
commit 74c3a86
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 70 deletions.
diff --git a/packages/components/paginator/src/paginator-page.scss b/packages/components/paginator/src/paginator-page.scss
@@ -12,44 +12,16 @@
   --_transition-easing: var(--sl-animation-button-easing);
 }
 
-button:focus-visible {
-  outline: var(--_focus-outline);
-  outline-offset: var(--_focus-outline-offset);
-  z-index: 1;
-}
-
-button:where(:active, :hover) {
-  transition-duration: var(--_transition-duration);
-  transition-property: background, border-color, color, outline-color;
-  transition-timing-function: var(--_transition-easing);
-}
-
-button {
-  background: var(--_background);
-  border: var(--_border-width) solid var(--_border-color);
-  border-radius: var(--sl-border-radius-button-lg);
-  cursor: pointer;
-  display: inline-flex;
-  font: var(--_font);
-  justify-content: center;
-  min-inline-size: var(--_min-width);
-  padding-block: var(--_padding-block);
-  padding-inline: 0;
-  position: relative;
-}
-
-:host(:not([disabled]):hover) {
+:host(:hover) {
   --_background: var(--sl-color-button-default-ghost-hover-background);
   --_border-color: var(--sl-color-button-default-ghost-hover-border);
   --_color: var(--sl-color-button-default-ghost-hover-foreground);
-  --_transform: var(--sl-transform-button-default-ghost-hover);
 }
 
-:host(:not([disabled]):active) {
+:host(:active) {
   --_background: var(--sl-color-button-default-ghost-active-background);
   --_border-color: var(--sl-color-button-default-ghost-active-border);
   --_color: var(--sl-color-button-default-ghost-active-foreground);
-  --_transform: var(--sl-transform-button-default-ghost-active);
 }
 
 :host([active]) {
@@ -58,16 +30,48 @@ button {
   --_color: var(--sl-color-button-default-ghost-selected-idle-foreground);
 }
 
-:host([active]:not([disabled]):hover) {
+:host([active]:hover) {
   --_background: var(--sl-color-button-default-ghost-selected-hover-background);
   --_border-color: var(--sl-color-button-default-ghost-selected-hover-border);
   --_color: var(--sl-color-button-default-ghost-hover-foreground);
-  --_transform: var(--sl-transform-button-default-ghost-hover);
 }
 
-:host([active]:not([disabled]):active) {
+:host([active]:active) {
   --_background: var(--sl-color-button-default-ghost-selected-active-background);
   --_border-color: var(--sl-color-button-default-ghost-selected-active-border);
   --_color: var(--sl-color-button-default-ghost-active-foreground);
-  --_transform: var(--sl-transform-button-default-ghost-active);
+}
+
+:host([disabled]) {
+  --_background: var(--sl-color-button-default-ghost-disabled-background);
+  --_color: var(--sl-color-button-default-ghost-disabled-foreground);
+
+  cursor: default;
+  pointer-events: none;
+}
+
+button {
+  background: var(--_background);
+  border: var(--_border-width) solid var(--_border-color);
+  border-radius: var(--sl-border-radius-button-lg);
+  cursor: pointer;
+  display: inline-flex;
+  font: var(--_font);
+  justify-content: center;
+  min-inline-size: var(--_min-width);
+  padding: var(--_padding-block) 0;
+  position: relative;
+  transition: background var(--_transition-duration) var(--_transition-easing);
+}
+
+button:focus-visible {
+  outline: var(--_focus-outline);
+  outline-offset: var(--_focus-outline-offset);
+  z-index: 1;
+}
+
+button:where(:active, :hover) {
+  transition-duration: var(--_transition-duration);
+  transition-property: background, border-color, color, outline-color;
+  transition-timing-function: var(--_transition-easing);
 }
diff --git a/packages/components/paginator/src/paginator-size.scss b/packages/components/paginator/src/paginator-size.scss
@@ -1,16 +1,11 @@
 :host {
-  --_color: var(--sl-color-text-default);
-  --_font: var(--sl-text-new-body-md);
-  --_font-weight: var(--sl-text-typeset-font-weight-icon-regular);
-  --_gap: var(--sl-space-paginator-gap);
-
   align-items: center;
-  color: var(--_color);
+  color: var(--sl-color-text-default);
   display: flex;
   flex-wrap: wrap;
-  font: var(--_font);
-  font-weight: var(--_font-weight);
-  gap: var(--_gap);
+  font: var(--sl-text-new-body-md);
+  font-weight: var(--sl-text-typeset-font-weight-icon-regular);
+  gap: var(--sl-space-paginator-gap);
   padding: 0;
   text-wrap: nowrap;
 }

diff --git a/packages/components/paginator/src/paginator-status.spec.ts b/packages/components/paginator/src/paginator-status.spec.ts
@@ -143,7 +143,7 @@ describe('sl-paginator-status', () => {
       expect(ariaLive).to.have.rendered.text('Currently shown from 1 to 15 of 209 items');
     });
 
-    it('should have a proper active page when set bigger than the last one', async () => {
+    it('should have set active page to the last one when the number set is bigger than the total number of pages', async () => {
       let itemsCounterLabel = el.renderRoot.textContent?.trim();
 
       expect(itemsCounterLabel).to.exist;

diff --git a/packages/components/paginator/src/paginator.scss b/packages/components/paginator/src/paginator.scss
@@ -27,17 +27,17 @@
 
 .container {
   align-items: center;
-  display: grid;
+  display: flex;
   gap: var(--_gap);
-  grid-template-columns: 1fr auto 1fr;
 
   // to make focus-ring visible with overflow hidden:
   margin: calc(var(--_focus-space) * -1);
+  min-inline-size: 0;
   padding: 0;
 }
 
 .container[mobile] {
-  display: block;
+  // display: block;
 
   // to align with changes made for focus ring visibility with overflow hidden:
   margin: var(--_focus-space);

diff --git a/packages/components/paginator/src/paginator.spec.ts b/packages/components/paginator/src/paginator.spec.ts
@@ -72,14 +72,14 @@ describe('sl-paginator', () => {
       await new Promise(resolve => setTimeout(resolve, 100));
     });
 
-    it('should have a proper active page when set smaller than 1', async () => {
+    it('should go to page 1 when set page is smaller than 1', async () => {
       el.activePage = -1;
       await el.updateComplete;
 
       expect(el.activePage).to.equal(1);
     });
 
-    it('should have a proper active page when set bigger than the last one', async () => {
+    it('should have set active page to the last one when the number set is bigger than the total number of pages', async () => {
       el.activePage = 100;
       await el.updateComplete;
 

diff --git a/packages/components/paginator/src/paginator.stories.ts b/packages/components/paginator/src/paginator.stories.ts
@@ -20,6 +20,11 @@ type Story = StoryObj<Props>;
 export default {
   title: 'Navigation/Paginator',
   tags: ['draft'],
+  parameters: {
+    viewport: {
+      defaultViewport: 'reset'
+    }
+  },
   args: {
     total: 100,
     itemsPerPage: 10,
@@ -34,11 +39,6 @@ export default {
       table: { disable: true }
     }
   },
-  parameters: {
-    viewport: {
-      defaultViewport: 'reset'
-    }
-  },
   render: ({ activePage, itemsPerPage, pageSizes, total }) => {
     return html`
       <sl-paginator
@@ -55,21 +55,19 @@ export const Basic: Story = {};
 
 export const Overflow: Story = {
   args: {
-    ...Basic.args,
     total: 900
   }
 };
 
 export const Mobile: Story = {
-  args: {
-    ...Basic.args,
-    activePage: 5
-  },
   parameters: {
     viewport: {
       defaultViewport: 'iphone5'
     }
   },
+  args: {
+    activePage: 5
+  },
   render: ({ activePage, itemsPerPage, pageSizes, total }) => {
     return html`
       <sl-paginator
@@ -83,26 +81,19 @@ export const Mobile: Story = {
 };
 
 export const PageSizeComponent: Story = {
-  args: {
-    ...Basic.args
-  },
   render: ({ pageSizes, itemsPerPage }) => {
     return html`<sl-paginator-size .pageSizes=${pageSizes} .itemsPerPage=${itemsPerPage}></sl-paginator-size>`;
   }
 };
 
 export const ItemsCounterComponent: Story = {
-  args: {
-    ...Basic.args
-  },
   render: ({ activePage, itemsPerPage, total }) => html`
     <sl-paginator-status .total=${total} .activePage=${activePage} .itemsPerPage=${itemsPerPage}></sl-paginator-status>
   `
 };
 
 export const All: Story = {
   args: {
-    ...Basic.args,
     total: 200
   },
   render: ({ activePage, itemsPerPage, pageSizes, total }) => {

diff --git a/packages/components/paginator/src/paginator.ts b/packages/components/paginator/src/paginator.ts
@@ -93,9 +93,11 @@ export class Paginator extends ScopedElementsMixin(LitElement) {
   override connectedCallback(): void {
     super.connectedCallback();
 
-    if (!this.itemsPerPage) {
-      this.itemsPerPage = this.pageSizes ? this.pageSizes[0] : 10;
-    }
+    // if (!this.itemsPerPage) {
+    //   this.itemsPerPage = this.pageSizes ? this.pageSizes[0] : 10;
+    // }
+
+    this.itemsPerPage ||= this.pageSizes?.[0] || 10;
 
     // const itemsPerPage = this.itemsPerPage ?? 10;
     this.#pages = Math.ceil(this.total / this.itemsPerPage);
@@ -130,7 +132,7 @@ export class Paginator extends ScopedElementsMixin(LitElement) {
 
     if (changes.has('itemsPerPage')) {
       const itemsPerPage = this.itemsPerPage ?? 10;
-      this.#pages = Math.ceil(this.total / itemsPerPage) || 1;
+      this.#pages = Math.ceil(this.total / itemsPerPage);
 
       this.#setCurrentlyVisibleItems();