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

fix(metadata-sidebar): Fix metadata sidebar ai capabilities #3681

Merged

Conversation

jankowiakdawid
Copy link
Contributor

@jankowiakdawid jankowiakdawid commented Sep 26, 2024

Fix from where we read if Metadata Sidebar should have AI capabilities turned on.

Screenshots

Screenshot 2024-09-26 at 15 01 16

Screenshot 2024-09-26 at 15 01 53

Screenshot 2024-09-26 at 15 02 50

Screenshot 2024-09-26 at 15 20 36

@jankowiakdawid jankowiakdawid requested a review from a team as a code owner September 26, 2024 13:04
@jankowiakdawid jankowiakdawid force-pushed the fix-metadata-sidebar-ai-capabilities branch from 758dc65 to 45852eb Compare September 26, 2024 13:18
wpiesiak
wpiesiak previously approved these changes Sep 26, 2024
onError,
isFeatureEnabled,
}: MetadataSidebarRedesignProps) {
function MetadataSidebarRedesign({ api, elementId, fileId, onError, isFeatureEnabled }: MetadataSidebarRedesignProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is isFeatureEnabled doing what useFeatureEnabled is doing? do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think so.

useFeatureEnabled is checking the features array. In our case we can check if metadata redesign is enabled, or if metadata aiSuggestions are enabled.

isFeatureEnabled is a prop that comes from host application in metadataSidebarProps

metadataSidebarProps: MetadataSidebarProps,

it's defined here

type ExternalProps = {
isFeatureEnabled: boolean,
selectedTemplateKey?: string,
templateFilters?: Array<string> | string,
};

maybe @bfoxx1906 or @jstoffan have more insight if it should be used 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Took a look, useFeatureEnabled isn't used anywhere else in BUIE. However, Seems like a use case for components that don't have the features array passed down, so it probably is needed here. Have we tested useFeatureEnabled works as expected here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, useFeatureEnabled works as expected. It's covered with a storybook story test (also in this PR). It was recommended by @jstoffan some time ago.

Now after re-reading this I understand initial confusion. As I mentioned isFeatereEnalbed is a prop indicating if user has Metadata Sidebar enabled for them.

@jankowiakdawid jankowiakdawid force-pushed the fix-metadata-sidebar-ai-capabilities branch 4 times, most recently from dcf111c to 07fe7d4 Compare October 4, 2024 14:58
@jankowiakdawid jankowiakdawid force-pushed the fix-metadata-sidebar-ai-capabilities branch from 07fe7d4 to 3c83453 Compare October 7, 2024 12:51
greg-in-a-box
greg-in-a-box previously approved these changes Oct 7, 2024
@jankowiakdawid jankowiakdawid force-pushed the fix-metadata-sidebar-ai-capabilities branch from 471e553 to 9dbe6b7 Compare October 7, 2024 21:09
@mergify mergify bot merged commit fd164ec into box:master Oct 7, 2024
6 checks passed
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.

5 participants