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.

Sign up for GitHub

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

Jump to bottom

feat(settings): Add Dropdown component #1373

Merged
merged 7 commits into from
May 12, 2021
Merged

feat(settings): Add Dropdown component #1373

merged 7 commits into from
May 12, 2021

Conversation

ConradJChan
Copy link
Contributor

@ConradJChan ConradJChan commented May 10, 2021 •
edited
Loading

TODO:

  • unit tests
  • cross browser testing

Listbox:
Screen Shot 2021-05-10 at 1 15 34 PM

jstoffan
jstoffan reviewed May 11, 2021
View reviewed changes
Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

Given that we still need to add unit tests, we may want to break the refactor into its own PR.

src/lib/viewers/controls/settings/Settings.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.scss Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsFlyout.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.scss Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsDropdown.scss Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsCheckboxItem.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/hooks/useClickOutside.ts Outdated Show resolved Hide resolved
jstoffan
jstoffan reviewed May 11, 2021
View reviewed changes
src/lib/viewers/controls/settings/SettingsMenu.tsx Outdated
activeItem.focus();
}
}, [activeItem, isActive]);
if (!isActive) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed? It doesn't look like it was present before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's needed because SettingsFlyout doesn't play nice with multiple instances sharing the same activeRect. We can look to revert this with a Flyout mixin like we discussed earlier

@ConradJChan ConradJChan changed the title feat(settings): Add Checkbox and Listbox components feat(settings): Add Dropdown components May 11, 2021
@ConradJChan ConradJChan changed the title feat(settings): Add Dropdown components feat(settings): Add Dropdown component May 11, 2021
@ConradJChan ConradJChan marked this pull request as ready for review May 11, 2021 22:43
@ConradJChan ConradJChan requested a review from a team as a code owner May 11, 2021 22:43
jstoffan
jstoffan previously approved these changes May 11, 2021
View reviewed changes
Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

LGTM, though I think we're on the very edge of reviewable in terms of lines changed here.

@mergify mergify bot merged commit 8b2044d into box:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstoffan jstoffan jstoffan approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ConradJChan @jstoffan