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: add flyout wc spec #3691

Closed
wants to merge 22 commits into from
Closed

Conversation

SethDonohue
Copy link
Contributor

@SethDonohue SethDonohue commented Aug 11, 2020

Description

Adds Flyout Web Component Spec

Motivation & context

FAST needs an Popover component as the Dialog does not currently cover cases where positioning is needed of a focusable floating container to display UI related to what the user is doing.

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch 2 times, most recently from 8da1a87 to 2289e08 Compare August 11, 2020 20:34
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch 3 times, most recently from 9effb6a to cb65b1c Compare August 11, 2020 20:50
specs/flyout.md Outdated Show resolved Hide resolved
specs/flyout.md Outdated Show resolved Hide resolved
specs/flyout.md Outdated Show resolved Hide resolved
specs/flyout.md Outdated Show resolved Hide resolved
specs/flyout.md Outdated Show resolved Hide resolved
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch 4 times, most recently from fde0ae9 to 329bd0d Compare August 14, 2020 19:20
specs/flyout/flyout.md Outdated Show resolved Hide resolved
@chrisdholt
Copy link
Member

This will close #3752

specs/flyout/flyout.md Outdated Show resolved Hide resolved
specs/flyout/flyout.md Outdated Show resolved Hide resolved
specs/flyout/flyout.md Outdated Show resolved Hide resolved
@SethDonohue SethDonohue marked this pull request as ready for review September 3, 2020 18:22
@stale
Copy link

stale bot commented Sep 20, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the warning:stale No recent activity within a reasonable amount of time label Sep 20, 2020
@stale stale bot closed this Oct 4, 2020
@SethDonohue SethDonohue reopened this Oct 6, 2020
@stale stale bot removed the warning:stale No recent activity within a reasonable amount of time label Oct 6, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch from 297a193 to 3e5ec69 Compare October 6, 2020 20:47
@SethDonohue SethDonohue changed the title feat: add flyout wc spec feat: add popout (aka flyout) wc spec Nov 5, 2020
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch from 3e5ec69 to fa9e419 Compare November 5, 2020 21:29
@SethDonohue SethDonohue force-pushed the users/v-sedono/add-flyout-spec branch from b94d24f to 7e0fe97 Compare April 26, 2021 22:09
@SethDonohue
Copy link
Contributor Author

Anchored region can do "absolute" or "fixed" positioning. I think we can elaborate on whether we want to present that choice in the api after we get an initial version in, but what do you see the default being? "fixed" breaks out of parent containers more easily but absolute doesn't detach from the anchor as often.

I am still not totally clear on the difference here. What do you think would be better to start with?
I fee like absolute is a good start because a flyout generally is expected to stay next to it's target. Like a tooltip in that way. Thoughts?

After our discussion elsewhere let's go with fixed for now, since it will break out of its parent containers more easily and act more like a document level popup/flyout.

@scomea does this satisfy your question and hold on this PR?

@scomea
Copy link
Collaborator

scomea commented Apr 26, 2021

@SethDonohue Yeah, the only thing I'm not clear about is how a user might configure the component to position itself based on available space (ie. below, but if if doesn't fit above)

@SethDonohue
Copy link
Contributor Author

SethDonohue commented Apr 28, 2021

@SethDonohue Yeah, the only thing I'm not clear about is how a user might configure the component to position itself based on available space (ie. below, but if if doesn't fit above)

@scomea I actually forgot I already added a responsive attribute back in to the Attributes section:
- 'responsive' - boolean, whether or not the positioning is responsive based on available space, defaults to true

To me this says that unless they set responsive to false, the flyout will update it's position based on available space, but that does not mean the user can chose where it goes based on available space, i.e. to the bottom only instead of left or right as well. Is that something we can do with anchored region?

Currently if I set position to top, and have responsive to true (default) then if there is not enough space on top it will move down, but it could also move left or right depending on available space. Do you think this is good enough for the initial version or should we set something up where the user can decide where it may go if it needs to move?

@EisenbergEffect
Copy link
Contributor

This PR is very old now. What is our plan? We should close, move to draft, or get this in soon.

@EisenbergEffect
Copy link
Contributor

One year old this week. /cc @chrisdholt

@chrisdholt
Copy link
Member

One year old this week. /cc @chrisdholt

This should be updated to align and leverage the Popover specification from Open-UI: openui/open-ui#355

Lots of research has now been gathered there and there is an editors draft available. We should update our spec accordingly from a naming standpoint as well as look to align the API to the current guidance there. At that point I think we can move ahead with implementation. Ideally, we should ensure we spell out in the spec that our preference is to use Popover when available in browser and fall back to our code implementation when not.

@SethDonohue do you want to rework this based on the above or close and we can look to kick off a new one given the deltas?

@SethDonohue
Copy link
Contributor Author

Let's have a new one kicked off when ready!

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.

9 participants