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

v11 proposed changes for React #4356

Closed
joshblack opened this issue Oct 16, 2019 · 12 comments
Closed

v11 proposed changes for React #4356

joshblack opened this issue Oct 16, 2019 · 12 comments
Labels
package: react carbon-components-react version: 11 Issues pertaining to Carbon v11

Comments

@joshblack
Copy link
Contributor

joshblack commented Oct 16, 2019

Disclaimer: this issue is meant for capturing breaking changes that we'd like to see changed in v11 to help address some of the consistency issues across components. None of these changes are final, or implemented, and are subject to change.

@joshblack joshblack added package: react carbon-components-react version: 11 Issues pertaining to Carbon v11 labels Oct 16, 2019
@joshblack
Copy link
Contributor Author

joshblack commented Oct 20, 2019

Proposal: Dialog

A dialog is a window overlaid on either the primary window or another dialog window.

WAI-ARIA Practices

This component would serve as a primitive for building windows that are overlaid over the primary window, or other dialog windows, that have a requirement where windows under the dialog are inert.

This could be used as a primitive to build accessible versions of our Modal component, alongside panels or pop-ups that often appear in a UI.

Component API

Inspired by Reach UI
Inspired by Chakra

function Usage() {
  const [isOpen, open, close] = useDialog();

  return (
    <>
      <button onClick={open}>Open</button>
      <Dialog isOpen={isOpen} onDismiss={close}>
        <p>Content</p>
      </Dialog>
    </>
  );
}

Dialog

Prop Type
isOpen bool
onDismiss func
children node

Questions

  • How do we guarantee that there is a button to dismiss the dialog so a screen reader won't get trapped?
  • How do we provide an inert polyfill or implementation that works cross-browsers?

@joshblack
Copy link
Contributor Author

Proposal: useNotification

This hook would provide a way to send notifications and manage the UX behavior of doing this, namely:

  • How many notifications are shown at a given point in time
  • Where do notifications appear on the screen (and how through animation)
  • How a notification gets dismissed
  • How notifications get buffered

API

function Usage() {
  const [send] = useNotification();

  useEffect(() => {
    // Send a notification through an effect
    send('Basic notification');
  }, [send]);

  // Send a notification through an event
  function onClick() {
    send('Notification from an event');
  }

  // Send a specific type of notification
  function onSuccess() {
    send('Success notification', {
      type: NotificationTypes.success,
    });
  }

  // Be notified when a notification is dismissed
  send('Some notification', {
    onDismiss(message) {
      console.log('Notification was dismissed');
    },
  });
}

@joshblack
Copy link
Contributor Author

Proposal: VisuallyHidden

Inspired by Reach UI

A helper component that can be used to provide guidance to users of screen reader software.

API

import { VisuallyHidden } from 'the-design-system';

function Usage() {
  return (
    <button>
      <VisuallyHidden>Save</VisuallyHidden>
      <SaveIcon />
    </button>
  );
}

@joshblack joshblack changed the title v11 Proposed Changes for React v11 proposed changes for React Oct 25, 2019
@joshblack
Copy link
Contributor Author

joshblack commented Oct 25, 2019

Proposal: Project Conventions

  • The className prop supplied to a component should always be applied to the containing node
BadGood
function MyComponent({ children, className }) {
  return (
    <div>
      <p className={className}>
        {children}
      </p>
    </div>
  );
}
function MyComponent({ children, className }) {
  return (
    <div className={className}>
      <p>{children}</p>
    </div>
  );
}
  • Do not expose class instance methods through refs or class refs, an example of this would be fileUploader.clearFiles() called from the ref passed into FileUploader
  • Do not provide accessible labels by default, for example, "Provide icon description" as evidenced in numerous components
  • All additional props are spread onto the root node

@joshblack
Copy link
Contributor Author

Proposal: Leverage rollup for component bundles

Historically, we've published our package using transpiled babel sources to support ESM, CommonJS, and UMD bundles. This proposal would aim to remove all filepath import paths and instead provide a single entrypoint for all components bundled through Rollup.

@joshblack
Copy link
Contributor Author

joshblack commented Oct 31, 2019

Proposal: Text Truncation

Would be great if we offered a text truncation helper that would apply our CSS classes under the hood. This might look like:

<TruncatedText value="Some string" />

This would apply our bx--text-truncate--front and bx--text-truncate--back helper classes depending on local. In addition, it should supply a tooltip that is triggered when a user interacts with the text

Some other names could be:

<Text truncate value="Some string" />
<TruncateText value="Some string" />
<TextTruncate value="Some string" />

API

One thing would be that we, most likely, wouldn't want to allow children here for the value as we could have to cloneElement and pass in the className we'd expect. Under the hood, we could offer props like as to change the underlying node (by default it'd be <span>) and spread props outside of value onto the node.

@vpicone
Copy link
Contributor

vpicone commented Nov 20, 2019

Not sure if there's a place for one off just bad props

Button hasIconOnly should just be true if no children are provided to the button and an icon is.

Icon-specific props a11y or otherwise could be passed to the icon in renderIcon

@joshblack
Copy link
Contributor Author

Proposal: Component QOL updates

Tag

Action Reference Status
Remove usage of title prop Link

@joshblack
Copy link
Contributor Author

Proposal: ref conventions

We currently treat refs in a number of ways and ultimately should consolidate how we use them. It appears that there are several use-cases for ref that we'd like to promote

  • Getting a containing node for measurement
  • Focus management
  • Retrieving values of an underlying input

Traditionally, ref would be expected to be placed on the node of interest in the component. However, we would run into a situation where the usage of ref can be misconstrued or made harder to understand due to the variety of ways we might want to use it.

To start off, we could take a look at Button. In this case, it seems reasonable that ref would refer to both the underlying <button> which also is the containing node.

If we look at Tabs, we could expect ref to point to the containing node. If we look at Tab, however, there might be some confusion. Ideally ref would point to the underlying role="tab" node. How could we support someone wanting to measure the size of the component and expects ref to point to that?

@vpicone
Copy link
Contributor

vpicone commented Dec 19, 2019

@joshblack I think it should always go to the node of interest. If someone needs to measure a container so precisely that the node of interest doesn’t suffice, they can access the parent node through the ref.

My guess is that this use case is so rare it’s not worth putting the ref on the outside and making folks dig into the component for the more common use case (access to the focus able element/element with .value)

@vpicone
Copy link
Contributor

vpicone commented Dec 19, 2019

As a developer, if I pass a ref to an input or any form Component, and that ref doesn’t have a .value, I’m going to be surprised.

@joshblack
Copy link
Contributor Author

Thanks all for weighing in! Going to integrate these suggestions into our roadmap and then out into planning issues 👀

@joshblack joshblack mentioned this issue Aug 30, 2021
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: react carbon-components-react version: 11 Issues pertaining to Carbon v11
Projects
None yet
Development

No branches or pull requests

3 participants