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-2269): Tooltip documentation #313

Merged
merged 26 commits into from
Jul 29, 2022
Merged

docs(PPDSC-2269): Tooltip documentation #313

merged 26 commits into from
Jul 29, 2022

Conversation

Srinivasan-Ramasamy
Copy link
Contributor

@Srinivasan-Ramasamy Srinivasan-Ramasamy commented Jul 25, 2022 •
edited
Loading

PPDSC-2269

What

  1. Tooltip documentation page UI - http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2269-tooltip-documentation/components/tooltip/

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:

image

Who should review this PR:

How to test:

URL: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2269-tooltip-documentation/components/tooltip/

@Srinivasan-Ramasamy Srinivasan-Ramasamy added docs Improvements or additions to documentation WIP Work in progress labels Jul 25, 2022
@newskit-engineering
Copy link
Collaborator

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

@Srinivasan-Ramasamy Srinivasan-Ramasamy added the ready for design review Please assist in getting this reviewed label Jul 26, 2022
@Srinivasan-Ramasamy Srinivasan-Ramasamy removed ready for design review Please assist in getting this reviewed WIP Work in progress labels Jul 27, 2022
@Srinivasan-Ramasamy Srinivasan-Ramasamy added the ready for review Please assist in getting this reviewed label Jul 27, 2022
baburay23
baburay23 previously approved these changes Jul 29, 2022
View reviewed changes
Xin00163
Xin00163 reviewed Jul 29, 2022
View reviewed changes
Copy link
Contributor

@Xin00163 Xin00163 left a comment

Choose a reason for hiding this comment

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

A small change

site/pages/components/tooltip.tsx
'Overrides the typographyPreset applied to the Tooltip panel.',
},
{
attribute: 'panel.paddingBlockStart',
Copy link
Contributor

@Xin00163 Xin00163 Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Missing panel.paddingBlockEnd and some details for type & description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Xin00163 , updated the changes.

and no details on type and description in doc-> https://nidigitalsolutions.jira.com/wiki/spaces/NPP/pages/3942350849/Tooltip+Component+-+Web+Documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Srinivasan-Ramasamy , those are normally autogenerated so Design probably didn't put it in the spec. In this case, paddingBlock and paddingInline have defaults, so we can't use commonLogicalProps. The type and description should be consistent with what's in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @Xin00163 for the details, I've missed this one. Done the changes 👍

@Srinivasan-Ramasamy
Copy link
Contributor Author

Thanks @Xin00163 and @baburay23 for reviewing the code 👍

@Srinivasan-Ramasamy Srinivasan-Ramasamy merged commit 3a120bb into main Jul 29, 2022
@Srinivasan-Ramasamy Srinivasan-Ramasamy deleted the docs/PPDSC-2269-tooltip-documentation branch July 29, 2022 16:11
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
@Srinivasan-Ramasamy
docs(PPDSC-2269): Tooltip documentation (#313)
f350649 
* docs(PPDSC-2269): Tooltip documentation ui.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xin00163 Xin00163 Xin00163 approved these changes

@baburay23 baburay23 baburay23 approved these changes

@jps jps Awaiting requested review from jps

@pdimova pdimova Awaiting requested review from pdimova

@mutebg mutebg Awaiting requested review from mutebg

@Vanals Vanals Awaiting requested review from Vanals

@evgenitsn evgenitsn Awaiting requested review from evgenitsn

Assignees
No one assigned
Labels
docs Improvements or additions to documentation ready for review Please assist in getting this reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Srinivasan-Ramasamy @newskit-engineering @Xin00163 @baburay23