Code Monkey home page Code Monkey logo

primitives's People

Contributors

andy-hook avatar benoitgrelard avatar brattonross avatar bukinoshita avatar cpmsmith avatar dependabot[bot] avatar djm avatar dlehmhus avatar dliu-workos avatar godhyeongman avatar itsfaqih avatar jjenzz avatar joaom00 avatar kombucha avatar lesha1201 avatar luisorbaiceta avatar mattandryc avatar mihaip avatar myrjola avatar nicholaschiang avatar norman-ags avatar onemen avatar raulburigo avatar robbtraister avatar rortan134 avatar rynobax avatar sandrina-p avatar sinchang avatar wallynm avatar yarnsphere avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

primitives's Issues

Address `ProgressBar` concerns

Pedro was finding it odd that he had to provide a value and a width to the progress bar. We have been discussing a few solutions to this:

  • Rename the component to Progress so it more closely matches the native Progress API/expectations.
  • Set a width on the progress bar by default. Obviously this comes with styling concerns. Need to discuss.

Determine appropriate guideline for handling `disabled` states

As captured in #175 (comment)

In my mind, I'm expecting people to be using our components and if they use the as prop for these kinds of things it would be in my mind to pass in a compatible component. Personally if they start do as div I think it's kinda on them to them make sure that their own custom button is accessible.

@benoitgrelard raises a valid concern. Do we want to try and support every case where users may or may not render the element that we want them to and then shim all of the missing behavior?

We seem to universally agree that the answer here is no. If they don't use a proper button, then it's on them to add the appropriate aria attributes, add missing keyboard event support, etc.

With that said, we do certain things internally that are technically handled by the DOM, such as checking if a control is disabled before responding to events. I think that this is nice because:

  1. it's dead simple and adds very little complexity/code
  2. it means one less thing for a consumer to need to think about
  3. makes our handling of the disabled prop consistent with other components that can't render buttons under the hood (menu items, listbox items, future components that are more complex and require us to bail on the DOM, etc.)

@benoitgrelard seems to feel strongly that we shouldn't do this, in keeping with the fact that we don't do all of the other things that would support non-buttons. So I want to open a discussion here to see if we can come to some agreement on this moving forward.

New component: Status

Using this name instead of AlertStatus since alert and status are separate ARIA roles, so this feels a bit more consistent. This can be used for any sort of status indicator, whether a simple line of text or a block with live content (such as a list of active users in a chat window). This should be a very small wrapper around Alert.

function Examples() {
  return (
    <Fragment>
      <Status isAtomic>
        <Badge>User is {signedIn ? 'online' : 'offline'}</Badge>
      </Status>

      // or

      <Status isAtomic as={Badge}>User is {signedIn ? 'online' : 'offline'}</Status>

      // or

      <Status>
        <ul aria-title="Active users">
          {activeUsers.map((user) => (
            <li key={user.id}>{user.name}</li>
          ))}
        </ul>
      </Status>
    </Fragment>
  )
}

Things we'd like to complete before release

Needs

  • Rename/migrate repo
  • Fix issue discovered in Portal
  • Close off any redundant Github issues

do these last

  • Rename all internal references to interop/interop-ui to radix
  • Rename all packages to @radix-ui/…

Wants

  • Change Popper to remove PopperContent part and simplify dependent primitive APIs
  • Remove DebugContext related code
  • Update primitives to use extendComponent where applicable
  • Remove polyfills
  • Code consistency pass: general prop naming, is/shouldprefixes, setters, onChange handlers, etc
  • Review our bundle sizes and improve where possible

Tooltip: add features and address bugs

From Colm's notes in Notion:

  • Support multi-line + children. Max-width prop?

  • Prevent opening on focus when focus comes from clicks on other elements)

  • Also the bug Jenna mentioned where it triggers on click sometime

Remove closed/composed exports

In a conversation with @colmtuite we've agreed there isn't much value in maintaining these. Our docs will have clear examples of how to compose each component so users can copy/paste.

We believe that most of the time users will need the open versions for styling purposes anyway.

Add dev warnings to ensure the right elements are used?

After a long discussion with @peduarte and @jjenzz today we thought one good thing we could do would be to add dev runtime warnings for the types of element used.

It's pretty trivial to do with functional refs and would slot right into any of our useComposedRefs without needing to create extra useRef().

https://codesandbox.io/s/dark-tdd-2c3le

This way we can add warnings about them diverging from the norm and reminding them everything they would be required to do to make sure it remains accessible.

Spruce up the docs

Myself and @benoitgrelard have done some work on the docs in the initial-content branch. At the moment, it's all just static content though. We need to bring it to life.

I've been taking notes to compile a to-do list as I go. I'll just whack them all in here and you can break them up as you see fit.

  • Maybe we should use MDX or something similar? Maybe we're fine with just JSX. I've no preference really, whatever you think is best.
  • Components in the live code block examples should render the primitives, not Radix.
  • Get icons working. I've added some icons to the Button and Input pages, but they're not rendering for some reason.
  • Generate page title, component title/name and description. Right now, this metadata is hard-coded. It should be stored somewhere so we can use it in multiple places.
  • Set component name and description as og metadata.
  • In the left sidebar navigation, I've added headings with a plus icon (to indicate a closed accordion pane). We need to add our Accordion component. The plus icon should transform: rotate(45deg) when it's active.
  • Prevent left sidebar from scrolling to top on page reload. You'll see this when you scroll down, then navigate to a different page.
  • Generate the next/previous links. Currently they're just static. They should link to the next/previous page dynamically.
  • Generate the right sidebar mini nav. Currently this is just static. We need some way to build these for each page? Or maybe just componentise it so we can add them quickly? idk.

Note: I've omitted thumbnails, hero images, and social images from the above, because I'm still thinking about how we should handle them. For the Button and Input docs pages, I've added a placeholder hero image; just ignore that for now.

Support external context manager for collections

A limitation of our current collection component is that it isn't currently possible to delegate control farther up the tree from where the component is initially registered. Here's a scenario where this would be quite useful:

Suppose we want to break Select into some lower level components, one of which might be Listbox (just the list of items that exists inside the SelectPopover). Listbox has its collection, so we call createCollection and get what we need. But say SelectButton or SelectPopover needs access to the collection data (which it will if we want to recreate our scroll-into-view behavior). We have to duplicate the collection in Select and now all of that work is done twice.

What would be nice is if we could have some way to delegate control to a context provider higher up in the tree when we create a collection. It's not immediately clear to me what this looks like, but I imagine we'd have to revisit the API.

I'm not 100% sure it's the same thing, but at a quick glance React Aria appears to have a strategy for delegating control of a list via its SelectionManager interface.

Thoughts or ideas? @benoitgrelard @jjenzz

[Announce] VoiceOver announces incomplete changes in some situations

Per #133 (comment):

Seems to work pretty well although didn't announce the full sentence in Safari + Voiceover

The closest issue we could find elsewhere suggests this is a Safari bug. https://core.trac.wordpress.org/ticket/36853

I tried implementing a similar empty string fix suggested in the WordPress bug, but that doesn't work as intended in NVDA when working with stacked notifications (as a toast likely will). This may be something we can do separately in a Status package where notifications are replaced completely instead of stacked, as that it primarily where the problem lies and is most likely to occur.

Referenced in #166

Improve naming of our exports and consuming the exports

After chatting with Pedro during his experience using the primitives he realised he pretty much always does:

import { Dialog as DialogPrimitive, styles } from 

So perhaps we should already export them named like this, seeing that people will build on top and most likely call it the same.

Then we chatted about the styles bit and thought perhaps this could be improved also by tacking it on statically like the parts DialogPrimitive.styles or even DialogPrimitive.Content.style, DialogPrimitive.Overlay.style, etc.

Thoughts??

cc @jjenzz @peduarte @chaance

Figure out why build doesn't work with wildcard paths in tsconfig

To help with DX we should be able to do the following in our paths map in tsconfig.json:

paths: {
	"@interop-ui/react-*": ["packages/react/*/src"],
	"@interop-ui/primitive-*": ["packages/primitives/*/src"],
	"@interop-ui/*": ["packages/core/*/src"],
	"*": ["node_modules/*"],
	"$test/*": ["test/*"]
} 

5cff558 (#38)

All of the TS imports work with this but for some reason the rollup build fails.

Slider has been broken since moving into new repo

Issue one

The thumb moves when clicking to one side of it:

pr

I think this is the code that is supposed to prevent that but the data-part-id attribute no longer exists:

image

Issue two

The thumb initialises outside the bounds of the slider:

image

It's expected to initialise as follows:

image

Flex: review gap "polyfill" approach

@jjenzz I accidentally included some additions in #16 to pull in what you had from the old repo for Flex, but I don't think it'll work here since we no longer assume children will have mt or ml props. Thoughts?

Missing react-polymorphic dependency when installing react-portal

@StephenHaney hit a wall when using the latest Portal component.:

image

Slack convo starts here: https://modulz.slack.com/archives/C012ED9375L/p1607674019236600

What we discovered:

  • react-utils doesn't list react-polymorphic as a dep
  • react-portal doesn't list react-polymorphic as a dep
  • not all primitives list react-polymorphic as a dep

Note: Anyone using components from the Design System or from Primitives, make sure to pin the versions in package.json. As things are moving fast there are a few breaking changes. I can go to each product and update later if needed 👍 /cc @vladmoroz @ssskram @lucasmotta

RFC: Global app providers

The time has come to visit the pros and cons of exposing one or more context provider that might be helpful for consumers when dealing with higher level state needed by our components. I keep mentioning it in one-off conversations, so I feel like we should bring it up and decide on a path forward once and for all.

Rationale

There are a number of existing and in-progress components that need to either read or write some state related to other components. We've managed to get around this using DOM APIs for the most part, but there are times when this is going to be insufficient, particularly when the React tree is a closer source of truth than the DOM tree.

In Reach UI we've historically opted to store global state in a property on window to simplify matters so things just work for consumers. This has obvious shortcomings, and I think by and large React devs are comfortable with the context API these days. It's not as new and mysterious as it was when Reach decided to go that route. That said, there is value in having things just work and I think we should keep that goal intact. Context would be opt-in, expose otherwise un-configurable state data (like tooltip timing), and solve some particularly tricky problems that users will likely run into as their app gets more complex.

Potential use cases

  • Tooltip: Similar to focus, tooltips are global in the sense that only one tooltip should be visible at a time. Interactions in other components will dictate whether or not tooltips are visible. Further, the delay timing for a tooltip to appear should be configurable, but preferably at the app level as a difference between instances would create a disjointed user experience. Context would likely simplify implementation of these and reduce the likelihood of bugs.
  • Announce: Currently handled without context, but if we could potentially simplify implementation a bit by keeping announcements in a context tree. It could also be helpful to be able to read whatever is in the global live regions from other components. Not convinced at this stage we need it, but worth exploring if we decide to go this route with other components.
  • DismissableLayer: I believe we'll keep this as an internal utility and avoid exposing it directly, but we could include a context provider for it in separate providers in the components that use it to simplify stacking. See #191 (comment)
  • FocusLock/FocusScope: Still a WIP, but I imagine this could benefit greatly with global context as well
  • ScrollArea: We need to disable pointer events everywhere except for the active scrollbar while a user is interacting with a scroll area. Unsure if context would be better than the current approach, but we could consider this. Also might be good for some props to be configurable globally similar to a the tooltip to encourage better user experiences (i.e., auto-hide timing)

Implementation and usage

A MenuButton component may read Tooltip context but need not require Tooltip as a dependency. As such, we probably need to expose various contexts as a separate package. I don't think each context should be its own package necessarily, but we could have a @interop-ui/react-context package that exposes individual contexts as separate files so that they are tree shakable. Each context could export a provider and a hook with the necessary setters and getters that can be used anywhere in the tree. We would use these internally with default values so ensure components still work without context.

So for a consumer, this might look something like this:

import * as React from 'react';
// import file directly for tree-shaking, but also exposed in an index file
import { TooltipProvider } from '@interop-ui/react-context/dist/tooltip';
import { DismissableLayerProvider } from '@interop-ui/react-context/dist/dismissable-layer';

function GlobalState({ children }) {
  return (
    <DismissableLayerProvider>
      <TooltipProvider appearanceDelay={400}>
        <SomeOtherProvider>{children}</SomeOtherProvider>
      </TooltipProvider>
    </DismissableLayerProvider>
  );
}

// Consuming:

import { useTooltipContext } from '@interop-ui/react-context/dist/tooltip';

function SomeComponent({ children }) {
  const { appearanceDelay } = useTooltipContext();
  React.useEffect(() => {
    // some effect that needs to sync with tooltip's delay for whatever reason
    window.setTimeout(() => {
      //...
    }, appearanceDelay);
    // ...
  }, [appearanceDelay])
  return (
    <div>
      {/* whatever */}
    </div>
  );
}

Switch component: input vs. button

Getting ready to hammer out the switch based largely on the API we landed on for checkbox and I wanted to see if anyone had strong feelings as to whether or not we should render a button or input by default for this component.

I tend to lean towards an input. I think the pattern followed in react-aria for this component makes a lot of sense. https://react-spectrum.adobe.com/react-aria/useSwitch.html

  1. Input holds a value, so form support is easier
  2. Setting type="checkbox" gives us all of the interaction events for free as if it were a button
  3. role="switch" overrides semantics in any event, negating any semantic advantage of a button
  4. Assistive tech that doesn't recognize role="switch" will treat it like a standard checkbox, which to me makes more sense than a toggle button as a fallback because we may want it to hold a value

Thoughts? Objections? I was hoping to get this PR in tomorrow so let me know before I dig in!

Go through each component and check for anything broken, API usage, etc

We need to make sure all of our components are working as expected

  • AccesibleIcon
  • Accordion
  • Arrow
  • AspectRatio
  • Avatar
  • Checkbox
  • Collapsible
  • Dialog
  • LiveRegion
  • Lock
  • Popper
  • Popover
  • Portal
  • Radio
  • Sheet
  • Slider
  • Switch 🚧
  • Tabs
  • ToggleButton
  • Tooltip
  • VisuallyHidden

Can't close menus in a Dialog

Select, Dropdown Menu, Right Click Menu won't close in a Dialog as expected.
Using Radix ("@modulz/primitives": "0.0.1-40"):

Here is a snippet for testing on https://radix.modulz.app/docs/dialog

() => {
  const [isOpen, setIsOpen] = React.useState(false);
  return (
    <>
      <Button onClick={() => setIsOpen(true)}>
        Open dialog
      </Button>
      <Dialog
        isOpen={isOpen}
        onClose={() => setIsOpen(false)}
        padding={100}
      >
        <DropdownMenu
          menu={<Menu><MenuItem label="Hey there" /></Menu>}
          button={<Button>Open menu</Button>}
        />
        <Input my={5} placeholder="Just an input" />
      </Dialog>
    </>
  );
};

Popover.Popper vs. Popover.Position

We have recently made a change to rename Popover.Position to Popover.Popper and Tooltip.Position to Tooltip.Popper.

The rationale behind the change was that it was always rendering a Popper ultimately so the props that they receive are more aligned with these.

That being said @chaance has since raised a point that for him it's more confusing than before.
I think the case we're discussing here in particular is Popover.Popper.

A few points for discussion:

  • Popper seems to have become the kinda de-facto name for basically an element positioned relative to a target (popper.js is probably the main reason from it, but there may be more prior history/art, not sure)
  • Popover and Popper can be seen somehow as interchangeable to some people
  • we could either look for a new name for Popper and therefore update that and then Popover.Popper to Popover.NewName and Tooltip.Popper to Tooltip.NewName
  • or we could find a new name for Popover to remove the confusion arising from Popover.Popper

RFC: ScrollArea component

So as I expected, I think scrollbars are going to be a bit of an undertaking. Stephen pointed out that the scope of what we have currently in the modulz repo is pretty limited to our needs for the app, and after doing some research I think there will be a number of edge cases options users will need to consider for more general usage.

The following is a write-up on how I think we might approach it. Feedback before we dig in too far is most appreciated.

  • If the root element has a computed stye that makes it resizable, we need to make sure scrollbars respond to it accordingly. This is probably fine as we'll be using resize observer internally.
  • I know we aren't supporting IE11 but I think this component will use some more modern features in general and we probably don't want to crash an app that doesn't support them. The easy route is to probably do some feature detection at the top level and just bail if something isn't supported rather than load the thing with polyfills. This should just fallback to native scrollbars.
  • I think it makes sense for consumers to have the option for scrollbar settings to be universal in many cases and local in others. I'm going to try this with a global provider where useful defaults can be configured at the top of the app tree. The consumer can choose to use this or not.
  • Below is a general idea of what the API might look like.
  • Behaviors will need to be responsive to computed styles, which have no easy way to observe for changes. Resize observer will help in most cases, but there will probably we some edge cases to consider. Going to try and keep things as simple as possible at first, but something to think about.
  • textareas may not be supported in the initial phase, there are some quirks there that might complicate things. We'll need to experiment with those to get things right
  • I'm not sure we should support custom scrollbars on the body element at all. This thing may or may not require some heavy perf optimization as is, and I'm finding lots of edge cases in other libs around customizing for body. Simplebar doesn't support this, and I dug into OverlayScrollbars to see how they do it and it's super gnarly. Generally speaking I think users should expect very normal scrolling behavior on the document body. The body of a complex app like Modulz would have its own separate scroll area anyway, and on a normal content site the scrollbars should just work. No one should need to customize those that badly IMO.
// Some proposed props may be configurable options at the top of the app tree
// May or may not be overridden by instance
// All with have defaults so that scrollbars work without any prop configuration.
type ScrollAreaProps = {
  /**
   * How visibility is determined for both horizontal and vertical scrollbars by default.
   *   - `"always-visible"` - Always visible, regardless over overflow
   *   - `"hidden"` - Always hidden, regardless over overflow
   *   - `"auto"` - Scrollbar is visible on when the scroll area has overflowing content in the
   *     direction of the axis
   */
  scrollbarVisibility: 'always-visible' | 'hidden' | 'auto';
  /**
   * Set overflow behavior for both x and y axis. Similar to CSS properties, but passing them as
   * props instead may be a better way to prevent the need to figure out computed styles.
   */
  overflowBehaviorX: OverflowBehavior;
  overflowBehaviorY: OverflowBehavior;
  /**
   * Describes when scrollbars should be visually hidden.
   *   - `"never"` - Never hide automatically
   *   - `"scroll"` - Hide automatically after scroll
   *   - `"leave"` - Hide automatically after the pointer leaves the scrollable area
   *   - `"move"` - Hide automatically after scroll and after the pointer stops moving.
   */
  scrollbarAutoHideBehavior: 'never' | 'scroll' | 'leave' | 'move';
  /**
   * Set the delay in milliseconds for auto-hiding the scrollbars. When `scrollbarAutoHideBehavior`
   * is set to `"never"` this prop has no effect.
   */
  scrollbarAutoHideDelay: number;
  /**
   * Whether or not the user can scroll by dragging.
   */
  scrollbarDragScrolling: boolean;
};
function Scaffold() {
  return (
    <ScrollArea>
      {/* The Position component is just used for the observers, probably don't need to expose it */}
      <ScrollArea.Position />
      <ScrollArea.ContentArea>
        {/* all content goes here */}
      </ScrollArea.ContentArea>
      <ScrollArea.ScrollbarX>
        <ScrollArea.Track>
          <ScrollArea.Button direction="forward" />
          <ScrollArea.Button direction="back" />
          <ScrollArea.Thumb />
        </ScrollArea.Track>
      </ScrollArea.ScrollbarX>
      <ScrollArea.ScrollbarY>
      <ScrollArea.Track>
          <ScrollArea.Button direction="forward" />
          <ScrollArea.Button direction="back" />
          <ScrollArea.Thumb />
        </ScrollArea.Track>
      </ScrollArea.ScrollbarY>
      {/* If an element is resizable, it'll need a resize handle */}
      {/* The logic here is connected to scroll logic so I think it makes sense to include here */}
      <ScrollArea.ResizeHandle />
    </ScrollArea>
  );
}

Textarea: Consider new features

From Colm's notes in Notion:

  • Min-height prop and max-height props?
  • Expand on type?

Could we add these simply without too much concern, or will these features be more involved than we might expect? Maybe we save this issue for last and come back to it if we have time before we decide to release.

Consider primtive factory function for better DX

Today @benoitgrelard and @colmtuite and I had discussed a potential API for consumers to quickly create and style any primitive with a factory-type Component function. The nice thing here is that we have a lot of components that really don't do much other than provide a generic wrapper that supports that as prop, so we duplicate tons of code and create way more packages than we really need. The consumer in this case can just say "give me a Text component base" by passing "Text" to the Component function, and we give them the proper primitive and the resets so they don't have to think about all of that.

Posting this here as a public record of our conversation and to capture the API we mocked up!

import { styled } from './config'
import { Component } from './radix'

const Text = styled(Component('Text'))((reset) => ({
    ...reset,
    color: 'hiContrast',
  }),
  {
    size: {
      1: {
        fontSize: 1,
      },
      2: {
        fontSize: 2,
      },
    },
  },
);

export default Text

Issues with our type definitions

A tester reported top issue in this sandbox: https://codesandbox.io/s/stitches-stuff-d0m44?file=/src/Dialog.tsx

After digging, it seems to be an issue around the types coming off of our modules.
As you can see things are coming out as any.

I did a bit more digging locally (in a local CRA) and found a few very strange things:

  • on codesandbox we get this:
    image
  • local CRA doesn't complain like that but there are still some issues
  • Dialog and parts still come in as any
  • styles come kinda typed ok, but digging into the built types reveal some weird stuff:
    image
    image
  • it's missing a few part names (trigger, close), and has old ones (inner). I have no idea where it's getting this from… I have done a fresh build and get this too 🤔
  • the Dialog and parts coming as any I think is due to this:
    image
  • This looks like it didn't resolve properly when building the type definitions, not sure why… but it seems that's what is invalidating the whole type.

Add reset stories to components

We need to make sure all of our components are working as expected with resets and minimal inline styles

  • AccesibleIcon
  • Accordion
  • Arrow
  • AspectRatio
  • Avatar
  • Badge
  • Blockquote
  • Box
  • Button
  • Card
  • Checkbox
  • Code
  • Collapsible
  • Combobox
  • Container
  • Dialog
  • Divider
  • Flex
  • Grid
  • Header
  • Image
  • Input
  • Link
  • Overlay
  • Popover
  • Radio
  • Sheet
  • Slider
  • Switch
  • Table
  • Tabs
  • Text
  • Textarea
  • Toast
  • ToggleButton
  • Tooltip
  • VisuallyHidden

Add stitches as dev dep and create styled stories for each component

  • AccesibleIcon
  • Accordion
  • Arrow
  • AspectRatio
  • Avatar
  • Badge
  • Blockquote
  • Box
  • Button
  • Card
  • Checkbox
  • Code
  • Collapsible
  • Combobox
  • Container
  • Dialog
  • Divider
  • Flex
  • Grid
  • Header
  • Image
  • Input
  • Link
  • Overlay
  • Popover
  • Radio
  • Sheet
  • Slider
  • Switch
  • Table
  • Tabs
  • Text
  • Textarea
  • Toast
  • ToggleButton
  • Tooltip
  • VisuallyHidden

Integrate static style extraction in all of our components

Description

The style extraction script requires an update to all of our component style object exports.

BEFORE

export const styles: PrimitiveStyles = {
	button: {
		// ... styles
	},
	'button.disabled': {
		// ... styles
	}
};

AFTER

export const styles: PrimitiveStyles = {
	[interopSelector('Button')]: {
		// ... styles
		'&:disabled': {
			// ... styles
		}
	},
};

The interopSelector function will return button when compiled for consumers so consumers can spread styles.button into their component. When we build the components for publishing, an EXTRACT_CSS environment variable makes the interopSelector function return [data-interop-button] and generates a styles.css file.

Todo

  • AccesibleIcon
  • Accordion
  • Arrow
  • AspectRatio
  • Avatar
  • Badge
  • Blockquote
  • Box
  • Button
  • Card
  • Checkbox
  • Code
  • Collapsible
  • Combobox
  • Container
  • Dialog
  • Divider
  • Flex
  • Grid
  • Header
  • Image
  • Input
  • Link
  • LiveRegion
  • Overlay
  • Popover
  • Radio
  • Sheet
  • Slider
  • Switch
  • Table
  • Tabs
  • Text
  • Textarea
  • Toast
  • ToggleButton
  • Tooltip
  • VisuallyHidden

Modularize Lock component

Ok, before I plunge deep into this task I wanted to take the time to give a bit of history and highlights the different concerns at play here. Then I have a rough plan for what to try now.

cc @jjenzz and @chaance, have a read, hopefully that gives you a bit more context.

History

So back in the days, when developing the original primitives, the Lock came about as the solution we needed to deal with everything a Dialog needs to do to be accessible. By extension later this was also applied to Popover and subsequently used by Menu and all its usages (DropdownMenu, ContextMenu, Select).

There might have already been some assumptions made at this level.

Most of the functionality was implemented as part of a lower level JS only createFocusTrap utility. This was in an effort to make sure none of this critical behaviour is react-specific as we expect to implement these components in other view layers in the future.

Features in question

As an example here's a list of what needs to be done for a Dialog to be accessible:

  • just before opening:
    • store previously focused element before opening
  • when opening:
    • make the content element itself focusable in case nothing is focusable inside (to make sure we can still trap focus)
    • hide everything outside from screen readers (aria-hidden), we use https://github.com/theKashey/aria-hidden for this
    • move focus to the first tabbable element inside the Dialog (or a more specific given element)
  • whilst opened:
    • trap focus inside the Dialog
    • prevent outside clicks
    • prevent outside scroll
    • listen for escape key to close
    • listen for clicks outside to close
  • when closing:
    • in most cases restore focus to previously focused element (or a more specific given element) — exceptions here are for example if clicks outside are not blocked, then we want to let the focus do its normal thing

Then, there is extra complexity added around the fact that these might be nested, ie. nested Popovers like in Figma or Modulz, or even a Popover inside a Dialog, or Select inside a Popover, inside a Dialog, etc…

For this we had a number of things going on:

  • a focus trap manager which ensured only one was active at a time, making it way simpler to ensure only one level would do its thing when pressing escape, clicking outside, trapping focus, etc.
  • some extra code in the react layer (Lock) to enable winding down a whole stack of Popovers (Locks) when clicking outside and not preventing outside clicks (again coming from the needs of Modulz/Figma type apps).

Now

It's clearer than ever now, that whilst most of these are to be turned on for a Dialog, for Popover and others some features might be configured differently.

Since trying to add the ability to not trap focus in some cases (some Popover), new things came up:

  • should dismiss when blurring out of it
  • should restore focus in normal tabbing order (even if inside a Portal somewhere else…)

Proposed approach

I suggest we try to untangle a few different things, which should hopefully help us with maintenance, readability, etc.
I think the hardest stuff we're going to face here is related to the nesting but also potentially race conditions once having separating stuff. I don't really have a plan regarding this, we'll have to see how it works and how we can make it work once split up.

For now, the things that need to be separate in different components I'd say are:

  • focus management
    • moving focus to first tabbable element OR element of choice
    • restoring focus to previously focused element OR element of choice
    • ability to not restore focus at all sometimes and let the browser do its thing
  • focus trapping
    • the approach we currently use works well using sentinel elements
  • prevent outside click (new component)
  • prevent outside scroll (new component, using react-remove-scroll already)
  • listen for escape key (new component, need to see how we do nesting, etc)
  • listen for click outside (new component, need to see how we do nesting, etc)
  • hide from screen readers (new component, using aria-hidden)

Future

I think also perhaps another thing we need to discuss is how much of these features are applicable to each type of components, especially when it comes down to Popover, Menu, Select, etc.

New component: MenuButton

AKA DropdownMenu in the old repo, but I favor MenuButton as it's a bit more explicit. Dropdown can also refer to a ContextMenu which has some nuanced distinctions. Both should compose an underlying Menu component.

[Context] Improve Tooltip Error Message when Arrow is outside Position

I was playing around with the Tooltip and I didn't think about the fact that the Arrow needed to be inside the Position Component.

I got this error:

Popper.Arrow must be used within Popper

image

Im not sure how doable this is, but it'd be great if the error message was:

Tooltip.Arrow must be used within Tooltip.Position

I think that would be a more helpful message since it represents what I've done. I understand that the Tooltip is built on top of the lower level Popper, but as I user I haven't imported or used that at all.

Cheers!

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.