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.

Sign up for GitHub

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

Jump to bottom

docs(PPDSC-2855) pagination documentation #694

Merged
merged 11 commits into from
Mar 14, 2023
Merged

docs(PPDSC-2855) pagination documentation #694

merged 11 commits into from
Mar 14, 2023

Conversation

RashikaNewsUK
Copy link
Contributor

@RashikaNewsUK RashikaNewsUK commented Mar 8, 2023 •
edited
Loading

PPDSC-2855

What

  1. Background - why this is needed
  2. What did you do
  3. What does the reviewers should expect

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@RashikaNewsUK RashikaNewsUK added the docs Improvements or additions to documentation label Mar 8, 2023
@RashikaNewsUK RashikaNewsUK requested a review from a team as a code owner March 8, 2023 12:29
@pp-serviceaccount
Copy link
Collaborator

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2855-pagination-documentation
Storybook: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2855-pagination-documentation/storybook

@RashikaNewsUK RashikaNewsUK added the ready for design review Please assist in getting this reviewed label Mar 9, 2023
@RashikaNewsUK RashikaNewsUK added ready for review Please assist in getting this reviewed and removed ready for design review Please assist in getting this reviewed labels Mar 13, 2023
JohnTParsons
JohnTParsons previously approved these changes Mar 13, 2023
View reviewed changes
mutebg
mutebg reviewed Mar 13, 2023
View reviewed changes
site/pages/components/pagination.tsx Outdated
Comment on lines 443 to 448
{
name: 'children',
type: ['Exclude', '<React.ReactNode>', 'undefined'],
description: 'Label and icon of the pagination item',
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see in the code, this prop is not required

site/pages/components/pagination.tsx Show resolved Hide resolved
site/pages/components/pagination.tsx Show resolved Hide resolved
site/pages/components/pagination.tsx Outdated
Comment on lines 281 to 283
element: 'pagination',
attribute: 'role',
value: 'navigation',
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not have a "role", by default it uses the nav HTML tag which has an internal role of navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check @mutebg

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just remove this item from the array

site/pages/components/pagination.tsx Outdated
{
element: 'link',
attribute: 'aria-current',
value: 'current',
Copy link
Contributor

Choose a reason for hiding this comment

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

the value here is page but is set only on the currently page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check @mutebg

site/pages/components/pagination.tsx Outdated
{
element: 'link',
attribute: 'aria-disabled',
value: 'false',
Copy link
Contributor

Choose a reason for hiding this comment

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

the value here is true but is set only when the button is disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check @mutebg

mstuartf
mstuartf reviewed Mar 13, 2023
View reviewed changes
site/pages/components/pagination.tsx Outdated
},
{
name: 'buildHref',
type: '(number) ⇒ string',
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing but below you are using => instead of ⇒.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed @mstuartf

site/pages/components/pagination.tsx Outdated
required: false,
},
{
name: 'eventContext = {}',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed @mstuartf

mutebg
mutebg reviewed Mar 14, 2023
View reviewed changes
site/pages/components/pagination.tsx Outdated
componentDefaultsKey="pagination"
meta={{
status: MetaStatus.Supported,
introduced: 'v5.6.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

this is from v7.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @mutebg

site/pages/components/pagination.tsx Outdated
Comment on lines 280 to 296
{
element: 'pagination',
attribute: 'nav',
value: 'navigation',
description: (
<>
Identifies major groups of links used for navigating through a
website or page content{' '}
<Link
href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Navigation_Role"
target="_blank"
>
Learn more about the navigation role at MDN Web Docs
</Link>
</>
),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that you don't need this, can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed @mutebg

@RashikaNewsUK RashikaNewsUK removed the ready for review Please assist in getting this reviewed label Mar 14, 2023
@RashikaNewsUK RashikaNewsUK merged commit aca8e29 into main Mar 14, 2023
@RashikaNewsUK RashikaNewsUK deleted the docs/PPDSC-2855-pagination-documentation branch March 14, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnTParsons JohnTParsons JohnTParsons approved these changes

@mutebg mutebg mutebg approved these changes

@mstuartf mstuartf Awaiting requested review from mstuartf

Assignees
No one assigned
Labels
docs Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RashikaNewsUK @pp-serviceaccount @mutebg @JohnTParsons @mstuartf