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

[Bug]: Pagination shows redundant information when pagesUnknown is set to true. #12252

Closed
2 tasks done
Akshat55 opened this issue Oct 6, 2022 · 7 comments · Fixed by #12302
Closed
2 tasks done

[Bug]: Pagination shows redundant information when pagesUnknown is set to true. #12252

Akshat55 opened this issue Oct 6, 2022 · 7 comments · Fixed by #12302
Labels

Comments

@Akshat55
Copy link
Contributor

Akshat55 commented Oct 6, 2022

Package

@carbon/react, carbon-components-react

Browser

Chrome, Firefox, Edge

Package version

v1.14

Description

In pagination component, when pagesUnknown prop has been set to true, the page number is shown twice.

e.g.
1 page 1
If I change the page number via select, the page number also updates as such:
2 page 2
3 page 3
...etc,

The first number is the page option selected from the select element. Then the same information is repeated in the following text.
Shouldn't it just be Page <select element with number> [TEXT (SELECT ELEMENT)]? Is there a reason for repeating the page number?

I'm not sure if this counts as a design defect, but this issue was raised in Carbon Angular, just forwarding it here so we know what steps to take.

Screenshots

image
image
image

Steps to reproduce

Go to the following storybook (props have been already set, so no changes required):

https://react.carbondesignsystem.com/?path=/story/components-pagination--default&args=pagesUnknown:true

Code of Conduct

61130061 added a commit to 61130061/carbon that referenced this issue Oct 14, 2022
According to carbon-design-system#12252, when pagesUnknown is , pagination render page
number as same as selector which does not make sense to render this information twice.
So, I change page number (pageText) to page label in front of
the selector which makes more sense and make pagination looks cleaner.

BREAKING CHANGE: typeof pageText change from function to string.
@tw15egan
Copy link
Member

tw15egan commented Oct 14, 2022

Also posted in #12302

I think this can be fixed just by passing down pageText={() => ''} to the Pagination component as-is. It seems like there is a slight styling issue in this scenario, but can be fixed by just adding the following CSS to _pagination.scss

.#{$prefix}--pagination__right .#{$prefix}--pagination__text:empty {
    margin: 0;
}

Screen Shot 2022-10-14 at 10 26 38 AM

@tw15egan
Copy link
Member

If we still want to keep the page text, we could modify the pageText default function, but it could still be overridden if anyone is passing in their own translation strings via pageText.

    pageText = (page) => {
      return `page  ${pagesUnknown ? '' : page}`;
    },

We would then add two opposite ternary renders inside pagination__right like so:

Before the Select

        {pagesUnknown ? null : (
          <span className={`${prefix}--pagination__text`}>
            {pagesUnknown ? pageText(page) : pageRangeText(page, totalPages)}
          </span>
        )}

After the Select

{pagesUnknown ? (
          <span className={`${prefix}--pagination__text`}>
            {pagesUnknown ? pageText(page) : pageRangeText(page, totalPages)}
          </span>
        ) : null}

Which gets us.
Screen Shot 2022-10-14 at 10 39 39 AM

Would probably need to do some style adjustments as well to get the text inside of the border instead of outside to the left

@61130061
Copy link
Contributor

@tw15egan

Add two opposite ternary renders inside pagination__right work for me.

About border, I already moved border-left from selector to pagination__right div which also work with code you suggested as well.

which gets us.
Screen Shot 2565-10-14 at 22 24 19
Screen Shot 2565-10-14 at 22 24 26

However, would be better if we use label tag instead of span? which is similar to ItemPerPageText, so user can click on the pageText to focus on the selector.

@61130061
Copy link
Contributor

@tw15egan

Like this.

{pagesUnknown ? (
  <label
    id={`${prefix}-pagination-select-${inputId}-right-label`}
    className={`${prefix}--pagination__text ${prefix}--pagination__page-text`}
    htmlFor={`${prefix}-pagination-select-${inputId}-right`}>
    {pagesUnknown ? pageText(page) : pageRangeText(page, totalPages)}
  </label>
) : null}

Just wonder. If you prefer span, I will go for it.

@tw15egan
Copy link
Member

@61130061 I believe we already have a hidden label for the select, so I'd just keep it a span to avoid redundant screenreader text.

Your examples look great 👍🏻

@61130061
Copy link
Contributor

@tw15egan All done! Thanks for your suggestions. 🙏🏻

@hudlow
Copy link

hudlow commented Oct 14, 2022

I'm confused. How do you populate the items in the select if the number of pages is unknown?

@tay1orjones tay1orjones added type: enhancement 💡 proposal: accepted This request has gone through triaging and we are accepting PR's against it. proposal: active development This request is actively being worked during the current sprint. and removed type: bug 🐛 status: needs triage 🕵️‍♀️ status: needs design triage 🕵️‍♀️ proposal: accepted This request has gone through triaging and we are accepting PR's against it. labels Oct 17, 2022
kodiakhq bot added a commit that referenced this issue Nov 15, 2022
…12302)

* fix(react/pagination): change style of pageText when pagesUnknown is

According to #12252, when pagesUnknown is , pagination render page
number as same as selector which does not make sense to render this information twice.
So, I change page number (pageText) to page label in front of
the selector which makes more sense and make pagination looks cleaner.

BREAKING CHANGE: typeof pageText change from function to string.

* fix(react/pagination): render page number twice modified

* Update packages/react/src/components/Pagination/next/Pagination.js

Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>

* fix(react/pagination): resolve another conflict on /Pagination.js

* chore: fix deleted files

Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
Co-authored-by: Taylor Jones <taylor.jones826@gmail.com>
Co-authored-by: TJ Egan <tw15egan@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@kodiakhq kodiakhq bot closed this as completed in #12302 Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants