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

feat(PPDSC-2176): feat-ppdse-2176-tooltip-updates #256

Merged
merged 18 commits into from
Jul 8, 2022

Conversation

baburay23
Copy link
Contributor

@baburay23 baburay23 commented Jul 4, 2022

PPDSC-2176

What

  1. Background - tooltip should still work when its children component has a disabled prop. Also add Tooltip to the theme checker
  2. What did you do- I noticed that material ui have a solution for this on their website https://mui.com/material-ui/react-tooltip/#disabled-elements.
    `<Tooltip
    content="You are not permitted to view this"
    placement="right"
    trigger={['focus', 'hover']}
    Button `

The above example will work, without using span a component with a disabled prop will not trigger user interactions so a Tooltip will not activate on normal events like hover.
I will also update the tooltip documentation here
i have also added a console.warn error so the user is alerted if the is not passed
5. What does the reviewers should expect- added a story example and test, also added Tooltip to theme checker

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:

@baburay23 baburay23 added the WIP Work in progress label Jul 4, 2022
@baburay23 baburay23 changed the title feat(PPDSC-2176): feat-ppdse-2176-added tooltip to themecheck feat(PPDSC-2176): feat-ppdse-2176-tooltip-updates Jul 4, 2022
@github-actions github-actions bot added the feature This change contains a new feature label Jul 4, 2022
@newskit-engineering
Copy link
Collaborator

@baburay23 baburay23 added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Jul 5, 2022
Xin00163
Xin00163 previously approved these changes Jul 6, 2022
@@ -109,6 +125,7 @@ export const StoryTooltip = () => (
>
<Button
size={ButtonSize.Small}
// disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

comment?

src/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
src/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
src/tooltip/__tests__/tooltip.test.tsx Outdated Show resolved Hide resolved
@baburay23 baburay23 requested a review from mstuartf July 7, 2022 10:38
@@ -32,6 +32,17 @@ const ThemelessTooltip: React.FC<TooltipProps> = ({

const contentIsString = typeof content === 'string';

const showSpanWarning = (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const showSpanWarning = (): void => {
const showDisabledWarning = (): void => {

mstuartf
mstuartf previously approved these changes Jul 7, 2022
@baburay23 baburay23 merged commit 977b835 into main Jul 8, 2022
@baburay23 baburay23 deleted the feat/PPDSC-2176-tooltip-updates branch July 8, 2022 13:19
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
* feat(PPDSC-2176): feat-ppdse-2176-added tooltip to themecheck

* feat(PPDSC-2176): feat-ppdse-2176-adding span

* feat(PPDSC-2176): feat-ppdse-2176-added console warn

* feat(PPDSC-2176): feat-ppdse-2176-added warn test

* feat(PPDSC-2176): feat-ppdse-2176-updated warning messaget

* feat(PPDSC-2176): feat-ppdse-2176-added production

* feat(PPDSC-2176): feat-ppdse-2176-updated tests

* feat(PPDSC-2176): feat-ppdse-2176-removed commeny

* feat(PPDSC-2176): feat-ppdse-2176-updated tests

* feat(PPDSC-2176): feat-ppdse-2176-changed useeffect

* feat(PPDSC-2176): feat-ppdse-2176-updated useeffect

* feat(PPDSC-2176): feat-ppdse-2171-chnaged func name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants