feat(boxai-sidebar): BoxAISidebar header added #3698
Conversation
|
Because this is a public repo, we should not paste Jira tickets. Shouldn't the "expand" button be right aligned? |
| @@ -15,3 +13,5 @@ export default { | |||
| token: global.TOKEN, | |||
| }, | |||
| }; | |||
|
|
|||
| export const basic = {}; | |||
There was a problem hiding this comment.
What is the reason for moving this line below default export?
There was a problem hiding this comment.
No exact reason, just looked at other tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
just to reiterate, the default export is usually on the bottom, so the basic export should be moved above it
|
Looks like style of the header is not matching other headers. It should be bold, also not sure about font size |
@MateuszMamczarz could confirm, that for now BoxAISidebarHeader should only have this Expand button on the left, near the Box AI title |
src/elements/content-sidebar/stories/tests/BoxAISidebar-visual.stories.tsx
Show resolved
Hide resolved
| @@ -15,3 +13,5 @@ export default { | |||
| token: global.TOKEN, | |||
| }, | |||
| }; | |||
|
|
|||
| export const basic = {}; | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
DanilaRubleuski
force-pushed
the
box-ai-panel-preview-header
branch
from
92883db
to
819b904
Compare
DanilaRubleuski
force-pushed
the
box-ai-panel-preview-header
branch
from
b9f2347
to
23a7549
Compare
| justify-content: left; | ||
|
|
||
| .bcs-title { | ||
| padding-right: $space-3; |
There was a problem hiding this comment.
Could probably use gap in this case?
| @@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
| defaultMessage: 'Modify General Task', | |||
| description: 'modal title for when editing an existing general task', | |||
| }, | |||
| expandBoxAI: { | |||
| id: 'be.expandBoxAI', | |||
There was a problem hiding this comment.
we should add be.contentSidebar to the id prefix since we dont want to risk any conflicts. i.e. in case some other part of the repo has the same message id - we can't have duplicates
| @@ -24,3 +24,12 @@ export const BoxAIInSidebar: StoryObj<typeof BoxAISidebar> = { | |||
| expect(sidebar).toBeInTheDocument(); | |||
| }, | |||
| }; | |||
|
|
|||
| export const ExpandButtonCheck: StoryObj<typeof BoxAISidebar> = { | |||
There was a problem hiding this comment.
i assume this test will become a visual regression for the entire expanded view in Box AI right? so maybe we can name it like BoxAIWithExpandedView
| @@ -15,3 +13,5 @@ export default { | |||
| token: global.TOKEN, | |||
| }, | |||
| }; | |||
|
|
|||
| export const basic = {}; | |||
There was a problem hiding this comment.
just to reiterate, the default export is usually on the bottom, so the basic export should be moved above it
| const expandButton = screen.getByRole('button', { name: 'Expand' }); | ||
| await userEvent.click(expandButton); | ||
|
|
||
| expect(mockOnExpandClick).toHaveBeenCalled(); |
There was a problem hiding this comment.
there can be issues and unexpected behavior if we forget to clear/reset the mock calls between tests. i think we handle this in a global configuration but just in case, it's better to not use reuse the fn mock for the expect so it's more intentional about what should happen
e.g. check for a separate prop (defined in the local scope) and/or explicitly add an afterEach
afterEach(() => {
jest.clearAllMocks();
});
// or
const onExpandClick = jest.fn();
renderComponent({ onExpandClick });
...
expect(onExpandClick).toHaveBeenCalled();
05cc563
to
f791408
Compare
DanilaRubleuski
requested review from
tjuanitas and
JChan106
and removed request for
greg-in-a-box
| @@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
| defaultMessage: 'Modify General Task', | |||
| description: 'modal title for when editing an existing general task', | |||
| }, | |||
| expandBoxAI: { | |||
| id: 'be.contentSidebarExpandBoxAI', | |||
There was a problem hiding this comment.
Can we namespace this to be similar to the other messages? Maybe be.contentSidebar.boxAI.expand?
| expandBoxAI: { | ||
| id: 'be.contentSidebarExpandBoxAI', | ||
| description: 'Default message for Box AI expand button in sidebar header', | ||
| defaultMessage: 'Expand', |
There was a problem hiding this comment.
Minor, but let's move this up a line to stay consistent with the other messages.
| @@ -117,6 +117,11 @@ const messages = defineMessages({ | |||
| defaultMessage: 'Modify General Task', | |||
| description: 'modal title for when editing an existing general task', | |||
| }, | |||
| expandBoxAI: { | |||
There was a problem hiding this comment.
boxAISidebarExpand seems accurate?
| } | ||
|
|
||
| function BoxAISidebar() { | ||
| function BoxAISidebar({ onExpandClick }: BoxAISidebarProps) { | ||
| const { formatMessage } = useIntl(); | ||
|
|
||
| return ( | ||
| <SidebarContent | ||
| className={'bcs-BoxAISidebar'} |
There was a problem hiding this comment.
Minor, but this can be a regular string: className="bcs-BoxAISidebar"
| <IconButton | ||
| onClick={onExpandClick} | ||
| icon={ArrowsExpand} | ||
| aria-label={formatMessage(sidebarMessages.expandBoxAI)} |
There was a problem hiding this comment.
We typically alphabetize our props whenever possible.
jstoffan
dismissed
greg-in-a-box’s stale review
Reviewer out of office, but all feedback appears to have been addressed.
DanilaRubleuski
force-pushed
the
box-ai-panel-preview-header
branch
from
6a2d980
to
97c08ca
Compare
DanilaRubleuski
dismissed stale reviews from tjuanitas and jstoffan
via
6d67640
Description
Added header to BoxAISidebar, which contains Expand button.
Screenshots