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(content-sidebar): Add placehoder metadatasidebar redesigned #3570

Merged

Conversation

jankowiakdawid
Copy link
Contributor

This PR introduces new placeholder component for work on redesigning Metadata Sidebar.
At current state this placeholder component doesn't offer much functionality nor look.
Screenshot 2024-07-03 at 23 51 03

The idea is to have this component to align and parallel work on upgrading MetadataSidebar to use Blueprint-web components and extends it's functionality with AI.

The component is being lazy loaded to not add to main bundle, and is hidden behind additional feature flag.

@jankowiakdawid jankowiakdawid requested review from a team as code owners July 3, 2024 21:53
@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch 3 times, most recently from 3d52c5e to 05df107 Compare July 4, 2024 13:35
@jankowiakdawid jankowiakdawid changed the title Add placehoder metadatasidebar redesigned feat(content-sidebar): Add placehoder metadatasidebar redesigned Jul 4, 2024
Copy link
Contributor

@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.

Do we have a feature branch we want to use? Or do we intend to merge to master until we're blocked by internal dependencies?

src/elements/content-sidebar/MetadataSidebarRedesigned.tsx Outdated Show resolved Hide resolved
src/elements/content-sidebar/MetadataSidebarRedesigned.tsx Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarPanels.js Outdated Show resolved Hide resolved
@jankowiakdawid
Copy link
Contributor Author

Do we have a feature branch we want to use? Or do we intend to merge to master until we're blocked by internal dependencies?

We don't have yet. I was hoping we can get this one to master and then spawn a feature branch for us to merge to. If that is not advised I'll create a new branch right away.

@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch from 05df107 to f611ea3 Compare July 15, 2024 16:21
src/constants.js Outdated Show resolved Hide resolved
@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch 2 times, most recently from 0772506 to 489d4dc Compare July 16, 2024 16:12
JChan106
JChan106 previously approved these changes Jul 16, 2024
Copy link
Contributor

@JChan106 JChan106 left a comment

Choose a reason for hiding this comment

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

lgtm, lets spawn a feature branch after merging as mentioned

tjuanitas
tjuanitas previously approved these changes Jul 17, 2024
Copy link
Contributor

@tjuanitas tjuanitas left a comment

Choose a reason for hiding this comment

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

lgtm

src/constants.js Outdated Show resolved Hide resolved
src/elements/content-sidebar/MetadataSidebarRedesign.tsx Outdated Show resolved Hide resolved
src/elements/content-sidebar/SidebarPanels.js Outdated Show resolved Hide resolved
@jankowiakdawid jankowiakdawid dismissed stale reviews from tjuanitas and JChan106 via ba48d9b July 17, 2024 14:56
@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch from 489d4dc to ba48d9b Compare July 17, 2024 14:56
@@ -74,6 +79,7 @@ const MARK_NAME_JS_LOADING_DETAILS = `${ORIGIN_DETAILS_SIDEBAR}${BASE_EVENT_NAME
const MARK_NAME_JS_LOADING_ACTIVITY = `${ORIGIN_ACTIVITY_SIDEBAR}${BASE_EVENT_NAME}`;
const MARK_NAME_JS_LOADING_SKILLS = `${ORIGIN_SKILLS_SIDEBAR}${BASE_EVENT_NAME}`;
const MARK_NAME_JS_LOADING_METADATA = `${ORIGIN_METADATA_SIDEBAR}${BASE_EVENT_NAME}`;
const MARK_NAME_JS_LOADING_METADATA_REDESIGNED = `${ORIGIN_METADATA_SIDEBAR_REDESIGN}${BASE_EVENT_NAME}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

ORIGIN_METADATA_SIDEBAR_REDESIGN no longer exists - was previously the same as ORIGIN_METADATA_SIDEBAR

Suggested change
const MARK_NAME_JS_LOADING_METADATA_REDESIGNED = `${ORIGIN_METADATA_SIDEBAR_REDESIGN}${BASE_EVENT_NAME}`;
const MARK_NAME_JS_LOADING_METADATA_REDESIGNED = `${ORIGIN_METADATA_SIDEBAR}${BASE_EVENT_NAME}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further consideration I decided to leave ORIGIN_METADATA_SIDEBAR_REDESIGN and apply it properly, so we can track the loading performance. I'm sure @jstoffan knows how.

@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch from 79e2a5b to 36f8857 Compare July 17, 2024 16:03
@jankowiakdawid jankowiakdawid force-pushed the add-placehoder-metadatasidebar-redesigned branch from 36f8857 to 8e3e443 Compare July 18, 2024 11:23
Copy link
Contributor

@JChan106 JChan106 left a comment

Choose a reason for hiding this comment

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

lgtm

@mergify mergify bot merged commit 6a0d7ee into box:master Jul 18, 2024
6 checks passed
@jankowiakdawid jankowiakdawid deleted the add-placehoder-metadatasidebar-redesigned branch July 19, 2024 10:45
kajarosz pushed a commit to kajarosz/box-ui-elements that referenced this pull request Aug 21, 2024
…#3570)

* feat(content-sidebar): Introduce redesigned MetadataSidebar placeholder

In this new placeholder component we will be composing new experience
from new components from shared-feature

* feat(content-sidebar): Use isFeatureEnabled utility

* feat(content-sidebar): Remove lint disabling comments

* feat(content-sidebar): Rename component to MetadataSidebarRedesign

* feat(content-sidebar): Rename constant value to match it's name

Also use constant in mock to show they connected

* feat(content-sidebar): Rename class name to follow SUIT naming

* feat(content-sidebar): Remove unneeded ref and constant

* feat(content-sidebar): Remove unneeded constant

* feat(content-sidebar): Add performance mark for MetadataSidebarRedesign
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.

4 participants