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

refactor: use store for dialog state in CardFooterInstanceActionButtons #5935

Merged
merged 10 commits into from
Aug 25, 2022

Conversation

pownkel
Copy link
Contributor

@pownkel pownkel commented Aug 18, 2022

Details

Use store state instead of component state to determine when the issue filing settings dialog is open

Motivation

We should avoid using component state where possible, because it can lead to tightly coupled components if we decide to split up a file. In this case, we currently need to pass callbacks to CardFooterMenuItemsBuilder to set the state of CardFooterInstanceActionButtons, which is a confusing flow of data and doesn't fit the flux paradigm.

Instead of a background store, this PR creates a view-side CardsViewStore with the same pattern as TabStopsViewStore, which will reset its state when the page is refreshed so the dialog doesn't stay open between refreshes.

Context

Continues a change from #5852. CardFooterInstanceActionButtons is not currently used anywhere in the UI, but will be wired up in a future PR.

We currently have another dialog, the Export Report dialog, which is controlled by component state instead of store state. We may want to refactor that in a future PR for consistency.

Pull request checklist

  • Addresses an existing issue: #0000
  • Ran yarn null:autoadd
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@pownkel pownkel requested a review from a team as a code owner August 18, 2022 01:01

import { AsyncAction } from 'common/flux/async-action';

export class DialogActions {
Copy link
Contributor

@dbjorge dbjorge Aug 24, 2022

Choose a reason for hiding this comment

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

I think moving this state to the background feels reasonable in general, but I'm hesitant to make a generic sounding DialogActions/DialogActionCreator/dialog/-namespaced messages unless we actually intend to move most/all dialog state management into one central place. Particularly, I think it would feel a bit confusing to have a DialogActions that is unrelated to the most interesting dialog in the app (the target page details dialog).

My preference would be to reuse the existing IssueFilingActionCreator and IssueFilingActionMessageCreator instead of creating new Dialog-* stuff. That would also help minimize logical dependencies from common/components to the details view.

Copy link
Contributor Author

@pownkel pownkel Aug 24, 2022

Choose a reason for hiding this comment

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

Per offline discussion with @waabid, ended up refactoring this PR completely to use a new view-side store instead of a background store to keep behavior the same on refresh. This also removes all the generic-sounding Dialog- objects.

I considered using IssueFilingActionMessageCreator to trigger the open/close dialog actions, but it looks like the existing openDialog function isn't called anywhere, and it seems odd for something called a MessageCreator to have functions that invoke actions without using messages. I created a separate CardsViewController to wrap CardsViewActions instead.

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

I just had one comment on high-level naming/placement, the individual code changes all LGTM otherwise!

@pownkel pownkel force-pushed the refactor-reflowable-cards branch from 75b6bae to ba006fc Compare August 24, 2022 19:54
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

LGTM! It feels a little off maybe that the flux stuff is mingled under a components/ directory, but that's consistent with the tab stops view so it makes sense to change them together in a separate PR if we want to change that structure

Copy link
Member

@waabid waabid left a comment

Choose a reason for hiding this comment

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

LGTM

@pownkel pownkel force-pushed the refactor-reflowable-cards branch from 2e048e5 to 59281d4 Compare August 24, 2022 23:54
@pownkel pownkel force-pushed the refactor-reflowable-cards branch from 59281d4 to 8c8edec Compare August 25, 2022 16:57
@pownkel pownkel merged commit 222d974 into microsoft:main Aug 25, 2022
@pownkel pownkel deleted the refactor-reflowable-cards branch August 25, 2022 17:10
pownkel added a commit that referenced this pull request Sep 16, 2022
#### Details

Replaces `CardKebabMenuButton` with CardFooterInstanceActionButtons`,
which will reflow the buttons currently in the kebab menu in the card
footer in both web and electron. GIFs below showing the card footer
reflow in action (copied from #5852)
![card footer buttons reflowing as window is
resized](https://user-images.githubusercontent.com/55459788/178857149-63590123-322d-4243-a4c1-cbcbe876ab9d.gif)
![demo of focus handling when issue filing settings is opened and
closed](https://user-images.githubusercontent.com/55459788/178857155-8de62725-cef2-4937-a9e1-b54e17747c79.gif)

Other changes in this PR:
- Initialize a CardsViewStore and relevant objects in DetailsView and
render-initializer
- Move rendering for the issue filing settings dialogue into a
higher-level component (`IssuesTable` in web and `TestView` in Electron)
to fix a bug where every card was rendering its own separate dialogue,
each with callbacks to focus a different button
- Add state relevant to the issue filing settings dialogue to
`CardsViewStoreData`

##### Motivation

Finishes addressing #5609 to add reflow behavior to the cards UI

##### Context

Continues work from #5852 and #5935

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: #5609
- [x] Ran `yarn null:autoadd`
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [x] (UI changes only) Added screenshots/GIFs to description above
- [x] (UI changes only) Verified usability with NVDA/JAWS
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.

3 participants