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

fix(PPDSC-2203): handling non text children in Paragraph #221

Merged
merged 10 commits into from
Jun 10, 2022

Conversation

mutebg
Copy link
Contributor

@mutebg mutebg commented Jun 2, 2022

PPDSC-2203

What

  1. Background - drop-cap on the paragraph component is broken when you use no-text only children like text <b>other</b> since for React treads that as multiple children
  2. What did you do -
    2.1. - Updated code so that handles multiple children, and adjusted the ScreenOnly content, to wrap only first word instead of the whole paragraph text
    2.2. - When the first child is not a text it will not show the drop-cap.
    2.3. - Added support for React.Fragment
  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:

@github-actions github-actions bot added the fix This change fixes a bug label Jun 2, 2022
@newskit-engineering
Copy link
Collaborator

@mutebg mutebg changed the title fix(PPDSC-2203): handle multiple children in paragraph fix(PPDSC-2203): handling non text children in Paragraph Jun 2, 2022
@mutebg mutebg added the ready for review Please assist in getting this reviewed label Jun 2, 2022
@mutebg mutebg marked this pull request as ready for review June 2, 2022 11:00
@mutebg mutebg added WIP Work in progress and removed ready for review Please assist in getting this reviewed labels Jun 6, 2022
@mutebg mutebg added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Jun 6, 2022
src/typography/paragraph/paragraph.tsx Outdated Show resolved Hide resolved
src/typography/paragraph/paragraph.tsx Outdated Show resolved Hide resolved

export const Paragraph: React.FC<ParagraphProps> = ({
children,
children: ch,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep "children", why are we using an alias such as ch? I think makes it bit less easy to understand what is it

</ParagraphDropCap>
{firstWord.slice(1)}{' '}
</span>
<ScreenReaderOnly>{firstWord}</ScreenReaderOnly>
Copy link
Contributor

@Vanals Vanals Jun 7, 2022

Choose a reason for hiding this comment

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

Can you please go through the "manual tests" relevant to this ticket? The different browsers and Screen reader testing 🤔 ?

I can see how the screen reader is not behaving anymore as it should. How was before did allow a smooth reading of the paragraph, not you need extra steps! I can show you. I wonder if the return of the Paragraph component could be simplified into something like

return (
    <ParagraphText>
      {useDropCap && (
        <>
          <span aria-hidden="true">
            <ParagraphDropCap overrides={overrides}>
              {firstWord[0]}
            </ParagraphDropCap>
            removeFirstLetter(children)
          </span>
            
          <ScreenReaderOnly>{children}</ScreenReaderOnly>
        </>
      )}
    </ParagraphText>
  );

In this way you won't need any extra ternary statement and the SR will read it properly. Happy to jump on a call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it a bit, let me know what you think

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.

could we have a storybook scenario? The rest looks good

mstuartf
mstuartf previously approved these changes Jun 7, 2022
mstuartf
mstuartf previously approved these changes Jun 8, 2022
): string => {
const [firstChild] = children;
if (typeof firstChild === 'string') {
return firstChild.split('')[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny but you can use .charAt for getting the first letter of a string like so
return firstChild.charAt(0);

): (React.ReactChild | React.ReactFragment | React.ReactPortal)[] => {
const [firstChild, ...rest] = children;
if (typeof firstChild === 'string') {
return [firstChild.split('').slice(1).join(''), ...rest];
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly I think we can use .substring(1) instead of firstChild.split('').slice(1).join('').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good ones, thanks

@mutebg mutebg requested review from evgenitsn and Xin00163 June 9, 2022 05:09
@mutebg mutebg merged commit f2e64d6 into main Jun 10, 2022
@mutebg mutebg deleted the fix/PPDSC-2203-paragraph-dropcap branch June 10, 2022 05:13
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
* fix(PPDSC-2203): handle multilpe children in paragraph

* fix(PPDSC-2203): fix css issue

* fix(PPDSC-2203): add comment

* fix(PPDSC-2203): address some of comments

* fix(PPDSC-2203): fix a11y issue in the paragraph

* fix(PPDSC-2203): add a story

* fix(PPDSC-2203): update unit tests

* fix(PPDSC-2203): fix coverage

* fix(PPDSC-2203): change from arrat to string methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix This change fixes a bug ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants