feat(content-sidebar): Option to render Box AI nav button disabled #3831
Conversation
| tooltip={ | ||
| (showOnlyBoxAINavButton && boxAIDisabledTooltip) || | ||
| intl.formatMessage(messages.sidebarBoxAITitle) | ||
| } |
There was a problem hiding this comment.
can boxAIDisabledTooltip be falsy? if not, can we just use a tenary?
There was a problem hiding this comment.
Yes, boxAIDisabledTooltip will be null if there is no tooltip to display.
There was a problem hiding this comment.
is the tooltip only available when showOnlyBoxAINavButton is true ?
There was a problem hiding this comment.
Yes, that is the current assumption. I've simplified it to ternary expression.
showOnlyBoxAINavButton ? boxAIDisabledTooltip : intl.formatMessage(messages.sidebarBoxAITitle)
| @@ -87,6 +98,57 @@ describe('elements/content-sidebar/SidebarNav', () => { | |||
| expect(wrapper.find(IconChatRound)).toHaveLength(0); | |||
| }); | |||
|
|
|||
| describe('should render box ai tab with correct disabled state and tooltip', () => { | |||
| const defaultTooltip = messages.sidebarBoxAITitle.defaultMessage; | |||
There was a problem hiding this comment.
we should hardcode the expected message instead of referencing the base message so that in case that someone accidentally changes the message itself, this test should break
There was a problem hiding this comment.
My understanding is that we intentionally use imported messages, so that the tests will not fail when the text changes. The goal of this test is to check if the tooltip is visible either with a given message or the default message, no matter what the default message actually is. I've seen numerous examples of such pattern in tests in BUIE and internal repos. Also there are currently no tests checking the tooltip message against a hardcoded string for any tab tooltip. Maybe we should create separate tests checking those? WDYT?
There was a problem hiding this comment.
we want the test to fail if the text has changed because its an intentional change, so it would be required to update the tests too. having to update the tests is verification for an intentional text change. there have been occasions where additional messages were changed by accident and they persisted all the way up to parent applications before it was caught.
currently, majority of the occurrences in buie are from older tests, all the new tests are hardcoding the strings for assertions. any tests that is currently using messages within a test to assert will be changed to hardcoded strings that correspond to the message defaultMessage, when we reach them during the enyzme to rtl migrations
for clarity, this is an example of what talking about in case what im asking is unclear
There was a problem hiding this comment.
The request was clear to me, thank you for additional explanation. I've updated tests to use hardcoded string in order to follow the current convention of not using message variables in tests.
| tooltip={ | ||
| (showOnlyBoxAINavButton && boxAIDisabledTooltip) || | ||
| intl.formatMessage(messages.sidebarBoxAITitle) | ||
| } |
There was a problem hiding this comment.
is the tooltip only available when showOnlyBoxAINavButton is true ?
| @@ -87,6 +98,57 @@ describe('elements/content-sidebar/SidebarNav', () => { | |||
| expect(wrapper.find(IconChatRound)).toHaveLength(0); | |||
| }); | |||
|
|
|||
| describe('should render box ai tab with correct disabled state and tooltip', () => { | |||
| const defaultTooltip = messages.sidebarBoxAITitle.defaultMessage; | |||
There was a problem hiding this comment.
we want the test to fail if the text has changed because its an intentional change, so it would be required to update the tests too. having to update the tests is verification for an intentional text change. there have been occasions where additional messages were changed by accident and they persisted all the way up to parent applications before it was caught.
currently, majority of the occurrences in buie are from older tests, all the new tests are hardcoding the strings for assertions. any tests that is currently using messages within a test to assert will be changed to hardcoded strings that correspond to the message defaultMessage, when we reach them during the enyzme to rtl migrations
for clarity, this is an example of what talking about in case what im asking is unclear
kkuliczkowski-box
force-pushed
the
content-sidebar/option-to-render-boxi-nav-button-disabled
branch
from
ce0a7c4 to
3b10e73
Compare
kkuliczkowski-box
force-pushed
the
content-sidebar/option-to-render-boxi-nav-button-disabled
branch
from
3b10e73 to
97083af
Compare
Description
Added option to display disabled tab nav item (button) without the corresponding sidebar.
This is used for Box AI Sidebar when the file is not compatible with AI features or there are permissions issues. In that case we want to not render Box AI Sidebar, however we still want to display the tab nav button with a proper tooltip.
This is controlled with
boxai.sidebarfeature.Screenshots