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(boxai-sidebar): Empty Sidebar for Box AI #3668

Merged

Conversation

DanilaRubleuski
Copy link
Contributor

@DanilaRubleuski DanilaRubleuski commented Sep 19, 2024

For now PR needs SidebarPanels.test.js to be fixed, to be reviewed fully

Description

Added empty sidebar for Box AI, so the next features could be added to this empty container in next PRs.

Screenshots

Screenshot 2024-09-19 at 12 00 53

Comment on lines 80 to 95
test('should not fetch the file data if the id has not changed', () => {
const wrapper = getWrapper({ fileId: '123' });
const instance = wrapper.instance();
const newProps = { fileId: '123' };

instance.fetchFile = jest.fn();
instance.setState({ view: 'activityFeed' });
instance.setState = jest.fn();

instance.componentDidUpdate(newProps);

expect(instance.fetchFile).not.toBeCalled();
expect(instance.setState).not.toBeCalled();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that an exact copy of the above test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolving this comment as I don't understand why do we need this duplicated test?

src/elements/content-sidebar/ContentSidebar.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/Sidebar.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarPanels.js Outdated Show resolved Hide resolved
i18n/en-US.properties Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

jankowiakdawid
jankowiakdawid previously approved these changes Sep 24, 2024
@DanilaRubleuski DanilaRubleuski requested a review from a team as a code owner September 24, 2024 14:20
@DanilaRubleuski DanilaRubleuski changed the base branch from box-ai-sidebar to master September 24, 2024 14:24
@DanilaRubleuski DanilaRubleuski dismissed jankowiakdawid’s stale review September 24, 2024 14:24

The base branch was changed.

@DanilaRubleuski
Copy link
Contributor Author

@greg-in-a-box could you please take another look?

src/elements/content-sidebar/ContentSidebar.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarNav.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarUtils.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/ContentSidebar.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarUtils.js Outdated Show resolved Hide resolved
Comment on lines 80 to 95
test('should not fetch the file data if the id has not changed', () => {
const wrapper = getWrapper({ fileId: '123' });
const instance = wrapper.instance();
const newProps = { fileId: '123' };

instance.fetchFile = jest.fn();
instance.setState({ view: 'activityFeed' });
instance.setState = jest.fn();

instance.componentDidUpdate(newProps);

expect(instance.fetchFile).not.toBeCalled();
expect(instance.setState).not.toBeCalled();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolving this comment as I don't understand why do we need this duplicated test?

src/elements/content-sidebar/__tests__/SidebarNav.test.js Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 611377c into box:master Oct 4, 2024
6 checks passed
@DanilaRubleuski DanilaRubleuski deleted the box-ai-panel-preview-empty-sidebar branch October 5, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants