Skip to content

feat(docgen): docgen sidebar base - #3484

Merged
mergify[bot] merged 1 commit into
box:masterfrom
rustam-e:docgen-sidebar-base
Apr 2, 2024
Merged

mergify[bot] merged 1 commit into
box:masterfrom
rustam-e:docgen-sidebar-base

Conversation

@rustam-e

@rustam-e rustam-e commented Jan 9, 2024 •

Copy link
Copy Markdown
Contributor

Added: a docgen sidebar

  1. loads if the previewed file is a docgen template
  2. renders list of tags
  3. includes empty, loading and error states
Screenshot 2024-01-29 at 14 51 56

@rustam-e
rustam-e requested review from a team as code owners January 9, 2024 17:57
@CLAassistant

CLAassistant commented Jan 9, 2024 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@rustam-e rustam-e changed the title Docgen sidebar base feat(docgen): docgen sidebar base Jan 10, 2024

@tjuanitas tjuanitas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

took a look but I don't have a lot of context on this feature. do you have a teammate that's able to review as well? also really important to get Preview's input

Comment thread src/constants.js Outdated
Comment thread src/elements/common/messages.js
Comment thread src/elements/content-sidebar/ContentSidebar.js Outdated
Comment thread src/elements/content-sidebar/ContentSidebar.js Outdated
Comment thread src/elements/content-sidebar/DocgenSidebar.js Outdated
Comment thread src/elements/content-sidebar/Sidebar.js Outdated
Comment thread src/elements/content-sidebar/SidebarNav.js Outdated
Comment thread src/elements/content-sidebar/__tests__/DocgenSidebar.test.js Outdated
Comment thread src/elements/content-sidebar/SidebarPanels.js Outdated
Comment thread src/elements/content-sidebar/Sidebar.js Outdated
@rustam-e
rustam-e requested a review from tjuanitas January 17, 2024 14:55
Comment thread src/elements/content-sidebar/DocgenSidebar.js Outdated
Comment thread src/elements/content-sidebar/__tests__/DocgenSidebar.test.js Outdated
Comment thread src/elements/content-sidebar/DocgenSidebar.js Outdated
Comment thread src/elements/content-sidebar/DocgenSidebar.js Outdated
@rustam-e
rustam-e requested a review from jackinf January 17, 2024 16:50
@rustam-e
rustam-e force-pushed the docgen-sidebar-base branch from 19e7007 to 56c7381 Compare January 18, 2024 15:44
@rustam-e
rustam-e marked this pull request as draft January 30, 2024 16:11
@rustam-e
rustam-e requested a review from jackinf February 21, 2024 14:24
@rustam-e
rustam-e marked this pull request as ready for review February 21, 2024 14:31
Comment thread src/elements/content-sidebar/DocGenSidebar/TagTree.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/NoTagsAvailable.tsx Outdated
@bfoxx1906 bfoxx1906 self-assigned this Feb 21, 2024
@bfoxx1906
bfoxx1906 requested review from bfoxx1906 and jackinf and removed request for jackinf February 21, 2024 18:52
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/Sidebar.js Outdated
Comment thread src/elements/content-sidebar/Sidebar.js Outdated
@rustam-e
rustam-e requested a review from bfoxx1906 February 27, 2024 13:28
@rustam-e
rustam-e force-pushed the docgen-sidebar-base branch from 9899463 to ed83ad1 Compare February 27, 2024 16:41
Comment thread src/elements/content-sidebar/Sidebar.js Outdated
Comment thread src/elements/content-sidebar/Sidebar.js
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx
Comment thread src/elements/content-sidebar/DocGenSidebar/TagTree.tsx
Comment thread src/elements/content-sidebar/__tests__/Sidebar.test.js Outdated
Comment thread src/elements/content-sidebar/SidebarNav.js Outdated
Comment thread src/elements/content-sidebar/SidebarUtils.js
@rustam-e
rustam-e requested a review from bfoxx1906 March 7, 2024 14:05

@bfoxx1906 bfoxx1906 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left one comment about using features. Other than that it should be good.

@rustam-e
rustam-e requested a review from bfoxx1906 March 11, 2024 14:54
const { file: prevFile, docGenSidebarProps: prevDocGenSidebarProps }: Props = prevProps;
// need to re-check if file is a docgen-template on file change
if (file.id !== prevFile.id && docGenSidebarProps?.enabled && docGenSidebarProps?.checkDocGenTemplate) {
docGenSidebarProps.checkDocGenTemplate(api, file, metadataSidebarProps.isFeatureEnabled);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we have an await here? Is checkDocGenTemplate async?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no that's not necessary, checkDocGenTemplate will reset the isDocgenTemplate prop which will be responsible for deciding whether to display or not display the sidebar and whether to navigate to the docgen tab

@bfoxx1906 bfoxx1906 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few more comments.

Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/NoTagsIcon.tsx
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/types.ts Outdated
imageTree: {},
},
});
const tagsToJsonPaths = (tags: DocGenTag[]): JsonPathsMap => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks like it could be extracted to a utils file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's only used in this file and nowhere else, that's why I put it next to its consumer. I'd leave it as it is until we need to use it in multiple places.

Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/Error.tsx Outdated
Comment thread src/elements/common/messages.js Outdated
Comment thread src/elements/common/messages.js Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.scss Outdated
clientName: CLIENT_NAME_CONTENT_SIDEBAR,
defaultView: '',
detailsSidebarProps: {},
docGenSidebarProps: { enabled: false },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the way the enabled property is being used seems similar to how the other tabs are using the has[...] props below. are we able to follow that pattern here?

@rustam-e rustam-e Mar 21, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here's the difference:
enabled determined if the docgen feature is enabled for user / enterprise - it is based on permissions and feature flags
isDocGenTemplate is determined by whether a file is a docgen template which is retrieved via checkDocGenTemplate

if both happen to be true, we can render the sidebar. I am calling that hasDocGen in the Sidebar component that passes it to SidebarPanels and SidebarNav and is used consistently with the other tabs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in other words it's not enough for a docgen feature to be enabled to dispaly the sidebar tab - the file also needs to be a template

Comment on lines +107 to +109
if (docGenSidebarProps.enabled) {
docGenSidebarProps.checkDocGenTemplate(api, file, metadataSidebarProps.isFeatureEnabled);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a network request in ContentSidebar to retrieve metadata, are we able to move some of the logic there so multiple calls aren't made?

I'm thinking something like:

// inside of ContentSidebar.js

fetchMetadataSuccessCallback = ({ editors }: { editors: Array<MetadataEditor> }): void => {
    const { docGenSidebarProps }: Props = this.props;
    const { setDocGenState }: DocGenSidebarProps = docGenSidebarProps;
    
    this.setState({ metadataEditors: editors });

    if (SidebarUtils.canHaveDocGenSidebar(this.props)) {
        setDocGenState(); // internal call to determine value of isDocGenTemplate
    }
};

this should allow the function below to become:

// inside of Sidebar.js

handleDocGenUpdate = (prevProps: Props) => {
    const { docGenSidebarProps, hasDocGen }: Props = this.props;
    const { docGenSidebarProps: prevDocGenSidebarProps }: Props = prevProps;

    const { isDocGenTemplate }: DocGenSidebarProps = docGenSidebarProps;
    const { isDocGenTemplate: prevIsDocGenTemplate }: DocGenSidebarProps = docGenSidebarProps;

    if (hasDocGen && isDocGenTemplate && isDocGenTemplate !== prevIsDocGenTemplate) {
        history.push(`/${SIDEBAR_VIEW_DOCGEN}`);
    }
};

my hope is that the existing logic in ContentSidebar will handle when files changes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't use the logic in ContentSidebar because it only fetches metadata in following case:
const canHaveMetadataSidebar = !isFeatureEnabled && SidebarUtils.canHaveMetadataSidebar(this.props);
which is confusing to me since it seems to be the opposite of metadata being enabled. So I didn't want to change the existing logic that would affect metadata tab (since I don't understand it) and created my own.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but if it is indeed a mistake then I can try updating that indeed - I raised it with the metadata team

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I discussed it with @bfoxx1906 and decided t o leave it as it is - the code in the current implementation will be removed once we retrieve information of whether a file is a DocGen template from backend - the logic in ContentSidebar is quite old and affecting state of that component and Metadata sidebar so I'd like to avoid modifying it since I'll have to revert back once the logic is no longer necessary.

Comment thread src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/TagTree.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/TagTree.tsx Outdated
Comment thread src/elements/content-sidebar/DocGenSidebar/messages.tsx
Comment thread src/elements/content-sidebar/DocGenSidebar/TagsSection.tsx Outdated
@rustam-e
rustam-e requested review from bfoxx1906 and tjuanitas March 22, 2024 15:47

@bfoxx1906 bfoxx1906 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My concerns have been addressed and we confirmed that a lot of the docgen logic will eventually be moved to the backed api. Make sure @tjuanitas 's comments are addressed and we should be good to go.

@rustam-e

Copy link
Copy Markdown
Contributor Author

I have added the acknowledgement of the cla agreement - not sure why it's hanging.

Screenshot 2024-03-27 at 18 15 40 Screenshot 2024-03-27 at 18 15 37

@rustam-e

Copy link
Copy Markdown
Contributor Author
Screenshot 2024-03-27 at 18 17 47

chore(docgen): handle undefined case

chore(docgen): update copy

chore(docgen): update comments

chore(docgen): update variable names

fix(docgen): fix icon path

fix(docgen): remove tag icon

fix(docgen): add default isDocgenTemplate value

fix(docgen): add a check for fetching docgen data

fix(docgen): update data map

fix(docgen): update mock response in test

fix(docgen): update component and tests to conform to new tags response

Update src/elements/content-sidebar/DocgenSidebar.js

Co-authored-by: Trevor <7311041+tjuanitas@users.noreply.github.com>

fix(docgen): update view const name

fix(dogen): update to use new camel case for the product name

fix(docgen): rename props name

fix(docgen): address code review comments

fix(docgen): fix prop

fix(docgen): address code review comments

fix(docgen): address code review comments

fix(docgen): update snapshots

fix(docgen): lint errors

fix(docgen): lint errors

fix(docgen): disable tab on file change

fix(docgen): remove vscode specific comment

fix(docgen): convert tag properties to camel case

fix(docgen): disable docgen tab on file move by default

fix(docgen): split sidebar into components, fix response parsing

fix(docgen): tests

fix(docgen): import path

fix(docgen): casing of import file

fix(docgen): move accidentally moved css file back

fix(docgen): styles and header

fix(docgen): input styles

fix(docgen): remove search input

fix(docgen): use react intl

fix(docgen): move tags section into component, use translated strings

fix(docgen): add the translation string

fix(docgen): update tests

fix(docgen): add loading and empty state

feat(docgen): align the empty and error states with designs

fix(docgen): update snapshots

fix(docgen): convert docgen sidebar to a functional component

fix(docgen): remove comments

fix(docgen): convert to typescript

fix(docgen): spacing

fix(docgen): tests

fix(docgen): autoimport

fix(docgen): use sass variables for colors and font sizes

fix(docgen): svg props, use existing loading state

chore(docgen): add error state test

fix(docgen): ts error

fix(docgen): error message

chore(docgen): update snapshot

chore(docgen): update response fields to snake case

feat(docgen): map tags to json paths and render a json tree, fix tests

fix(docgen): use translations

fix(docgen): add translation strings

fix(docgen):  tests to use translated strings

refactor(docgen):  move metadata fetching logic out of BUIE

fix(docgen): check if docgen tempalte only if feature is enabled

fix(docgen): run the is docgen check on prop change

fix(docgen): make props optional, remove duplicate props

fix(docgen): address some of the code review comments

fix(docgen): address some of the code review comments

fix(docgen): add tests to sidebar

fix(docgen): add tests to sidebar nav

fix(docgen): add tests to sidebar utils

fix(docgen): update copy

fix(docgen): update translations

fix(docgen): update json tag conversion logic

fix(docgen): remove test data

fix(docgen): remove test data

fix(docgen): update tests with fixed nesting

fix(docgen): move docgen props into docGenSidebarProps property

fix(docgen): ts warnings

fix(docgen): address code review comments

fix(docgen): update snapshot

fix(docgen): address more code review comments

fix(docgen): address code review comments

fix(docgen): address code review comments

fix(docgen): use suit convention

fix(docgen): use different icon

fix(docgen): use suit convention

fix(docgen): remove unnecessary optional checks, add default value

fix(docgen): update css class name

fix(docgen): display error state

fix(docgen): test

fix(docgen): fix styles

fix(docgen): update metadata

fix(docgen): address more code review comments

fix(docgen): move mocks into a mock file

fix(docgen): rename the prop in sidebar nav
@rustam-e
rustam-e force-pushed the docgen-sidebar-base branch from 12499ce to 2ce1734 Compare April 2, 2024 18:02
@mergify
mergify Bot merged commit 4b18b21 into box:master Apr 2, 2024
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