Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Fix the thread flyout behavior #2937

Merged
merged 5 commits into from
Apr 27, 2018
Merged

Conversation

gdad-s-river
Copy link
Contributor

@gdad-s-river gdad-s-river commented Apr 21, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • hyperion (frontend)

Release notes for users (delete if codebase-only change)

  1. (fix for #2917) — Wrapped Flyout and Thread Settings Icon Button with React Popper to improve [and handle well] cutting out of dropdown due to scrollable container (react popper automatically switches its position from top ↔️ bottom); removed unnecessary styles to complement this change.

  2. (fix for https://github.com/withspectrum/spectrum/issues/2917#issuecomment-382850059) — Wrapped Flyout with OutsideClickHandler to handle click outside of it (to handle its closing), instead of setting a transparent div over the whole page to handle closing of the dropdown.

Gif of improved behavior

Small [issue unrelated] typo fix

  • Typo correction (Uppercase to Lowercase) of package name in package.json

…issuecomment-382850059

1. Typo correction (Uppercase to Lowercase) of package name in package.json
2. Wrapped Flyout and Thread Settings Icon Button with React Popper to handle cutting out of dropdown due to scrollable container; removed unnecessary styles to complement this change.
3. Wrapped Flyout with OutsideClickHandler to handle click outside of it (and handle its closing), instead of setting a transparent div over the whole page to handle closing of the dropdown.
@gdad-s-river gdad-s-river changed the title fix for #2917 + https://github.com/withspectrum/spectrum/issues/2917#… fix for #2917 Apr 21, 2018
@gdad-s-river
Copy link
Contributor Author

@brianlovin. Hello! I could use some help to understand what is happening here, namely why ci tests are failing, and the 'expected' awaiting status.

@brianlovin
Copy link
Contributor

@gdad-s-river the ci failing should have been fixed with #2930 - any chance you could pull the latest alpha code and re push to this PR to trigger a new build?

@gdad-s-river
Copy link
Contributor Author

@brianlovin I've a small question. Using outSideClickHandler does one think differently —

One hits the delete thread option ▶️ modal window opens ▶️ user hits cancel, then since it's outside of the Flyout, the flytout hides (which is not the case when a full screen extra div is used to handle outsideclick). Even though there are not tests regarding this, i wanted to ask what to do about this behavior.

@brianlovin
Copy link
Contributor

@gdad-s-river that's just fine for it to close behind the modal if a person selects 'delete' in the dropdown :)

@brianlovin
Copy link
Contributor

Looking through the CI failures now, it looks like 7 of them are related to the action bar, which makes me think they might be failing because of the changes here. If you do yarn run cypress:open and run the action_bar_spec.js can you see if those are failing on your end?

@brianlovin
Copy link
Contributor

@gdad-s-river I pulled your branch and made a couple small changes to fix the tests and the overall behavior of the flyout (we don't want it to close immediately when the user clicks on something - we have loading states and confirmation states for things like locking the thread, which are useful bits of feedback to users). Anyways, everything is now passing for me locally, we'll see how things hold up here in CI!

@brianlovin brianlovin changed the title fix for #2917 Fix the thread flyout behavior Apr 27, 2018
@brianlovin
Copy link
Contributor

All tests pass! I'm going to get this on alpha.spectrum.chat right now :)

@brianlovin brianlovin merged commit 680d282 into withspectrum:alpha Apr 27, 2018
@gdad-s-river
Copy link
Contributor Author

gdad-s-river commented Apr 28, 2018

@brianlovin oops, I have no idea how I missed your comment notifications 😅. I had fixed the CI issues 😁. But thank you for doing it yourself! This was my first experience seeing what end to end tests look like, and how they could be so beneficial to enforce required behaviour. I learnt a lot. Thanks to @FezVrasta for helping me out with finding an issue and generously putting time to help me figure things out when I was struggling with React Popper.

Working on this issue, I made a small video explaining a small component OutsideClicker, which sparked this twitter thread which ultimately (again thanks to another generous person Timur), after numerous twitter DMs led to this codesandbox.

I look forward to contributing more, because spectrum.chat is awesome!

@brianlovin
Copy link
Contributor

💜💜

Thanks @gdad-s-river - this is live by the way!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants