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

useInnerText; Fix EuiListGroupItem title attrs #2100

Merged
merged 18 commits into from
Jul 15, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 9, 2019

Summary

Adds a custom hook (useInnerText), a render prop component (EuiInnerText, for class components), and uses the former to fix #2011.

EuiListGroupItem would provisionally get a title attribute if it was suspected that its content would truncate. This was unnecessary if it used custom tooltips or was allowed to wrap lines. Logic added fixes this.
EuiListGroupItem also accepts React node children, which would make the rendered value of the title attribute unreadable ([object Object]). useInnerText now updates the attribute value to match the text content of the list item.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs

  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios

- [ ] This was checked against keyboard-only and screenreader scenarios
- [ ] This required updates to Framer X components

expect(component).toMatchSnapshot();
});

// test('uses innerText', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on getting these types of tests to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still stumped. I'm not actually sure if Enzyme can handle it.

@cchaos
Copy link
Contributor

cchaos commented Jul 9, 2019

Can you add this to the EuiFilterButton component as well?

@thompsongl
Copy link
Contributor Author

Ok, ready for review (sorry; should've made it Draft to start)

I'll figure out how to correctly test hooks in the meantime.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Can you alter the docs for innerText a bit?

  1. I'm thinking the big Usage section isn't really needed because the snippets should take care of that. (You can also reference the snippet tab in the explanation).
  2. I'd add a bit about why a consumer would need this or really our typically usage for it (truncation).
  3. Update the examples so it's a little clearer what you are trying to show.

Something like:

src-docs/src/views/inner_text/inner_text_example.js Outdated Show resolved Hide resolved
src/components/filter_group/filter_button.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Show resolved Hide resolved
src-docs/src/views/list_group/list_group_example.js Outdated Show resolved Hide resolved
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

A few more docs comments

src-docs/src/views/list_group/list_group_extra.js Outdated Show resolved Hide resolved

<EuiSpacer />

<EuiListGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these other two can be combined into one group even though it will still show a tooltip on the wrapped lines one.

  • First item
  • Second item
  • Third very, very long item that will surely force truncation
  • Fourth very, very long item with wrapping enabled that will not force truncation

Copy link
Contributor

Choose a reason for hiding this comment

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

TY! Just one more thing, I promise...

Can you add two snippets for this example?

<EuiListGroup showToolTips>...</EuiListGroup>
<EuiListGroupItem
  wrapText
  label={label}
/>

Copy link
Contributor

Choose a reason for hiding this comment

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

@thompsongl Did you see my last comment on here? ^^

src-docs/src/views/inner_text/inner_text.js Outdated Show resolved Hide resolved
src-docs/src/views/inner_text/inner_text_example.js Outdated Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

chandlerprall commented Jul 10, 2019

Love the simplicity!

This doesn't account for data that updates. Let's throw a mutation observer on that ref as well to catch text changes. Unfortunately we can't use EuiMutationObserver because we aren't wrapping, but should be pretty straightforward none-the-less.

@chandlerprall
Copy link
Contributor

This also doesn't update if the ref changes (important for re-attaching a mutation observer as well). This is a stupid bit of code, but this modified doc example demonstrates:

export default () => {
  const [[thing, type], setThing] = useState(['Input 1', 'span']);
  useEffect(() => {
    console.log('setting timeout');
    setTimeout(() => {
      console.log('setting to Input 2');
      setThing(['Input 2', 'div']);
    }, 5000);
  }, [setThing]);

  return (
    <EuiText size="s">
      <h5>Example:</h5>
      <EuiInnerText>
        {(ref, innerText) => (
          <React.Fragment>
            <EuiFlexGroup>
              <EuiFlexItem grow={false}>
                <EuiPanel paddingSize="s" grow={false}>
                  {React.createElement(
                    type,
                    {
                      ref,
                      title: innerText,
                    },
                    thing
                  )}
                </EuiPanel>
              </EuiFlexItem>
            </EuiFlexGroup>
            <h5 className="eui-displayInlineBlock">Output:</h5>{' '}
            <EuiCode>{innerText}</EuiCode>
          </React.Fragment>
        )}
      </EuiInnerText>
    </EuiText>
  );
};

with useInnerText modified with two console.logs

export function useInnerText(
  innerTextFallback?: string
): [RefObject<HTMLElement>, string | undefined] {
  const ref = useRef<HTMLElement>(null);
  const [innerText, setInnerText] = useState(innerTextFallback);
  useEffect(() => {
    console.log(ref.current);
    if (ref && ref.current) {
      console.log('setting innerText', ref.current.innerText)
      setInnerText(ref.current.innerText);
    }
  }, [ref.current]);

  return [ref, innerText];
}

console output is

inner_text.tsx:17 <span title=​"Input 1">​Input 1​</span>​
inner_text.tsx:19 setting innerText Input 1
inner_text.js:19 setting timeout
inner_text.tsx:17 <span title=​"Input 1">​Input 1​</span>​
inner_text.tsx:19 setting innerText Input 1
inner_text.js:21 setting to Input 2

the inner_text.tsx:17 <span title=​"Input 1">​Input 1​</span>​ lines are the first console.log in useEffect, but there isn't a subsequent one when the underlying element switches from a span to a div.

@thompsongl
Copy link
Contributor Author

@chandlerprall I stopped using useRef based on advice in this article. That solves the changing element problem.
Also instituted MutationObserver for the content updates, but the polyfill we use for testing doesn't jibe with useState. Commented out the test, but you can see it work in the docs now.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; tested examples locally; this is so cool ❄️

@thompsongl thompsongl merged commit abf4d52 into elastic:master Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiListGroupItem HTML title bug
3 participants