New: version history in details tab #220

DanDeMicco · 2018-03-14T20:44:10Z

This adds the versions API, and version history to the details tab. Relies on box-react-ui 22.6.0

boxcla · 2018-03-14T20:44:14Z

Verified that @DanDeMicco has signed the CLA. Thanks for the pull request!

priyajeet · 2018-03-14T21:01:26Z

src/flowTypes.js

@@ -338,3 +339,19 @@ export type MultiputPart = {
 export type MultiputData = {
    part?: MultiputPart
 };
+
+export type Version = {


There is a BoxItemVersion
Can you check if its duped.

It is similar, but it appears the BoxItemVersion has an incomplete type definition and also includes less fields, according to the sample api docs

I would refactor and make them the same, you would just need to add ? to properties that are optional and not needed for the object to be partial and just null check them. Should ideally not have two definitions of versions.

Combined the types. As far as I could tell from the api docs only 3 properties were mandatory

priyajeet · 2018-03-14T21:02:31Z

src/constants.js

@@ -25,6 +25,7 @@ export const VIEW_UPLOAD_SUCCESS: 'upload-success' = 'upload-success';
 export const TYPE_FOLDER: 'folder' = 'folder';
 export const TYPE_FILE: 'file' = 'file';
 export const TYPE_WEBLINK: 'web_link' = 'web_link';
+export const TYPE_VERSIONS: 'versions' = 'versions';


There shouldn't be a type versions, since thats item type, and thats only the above 3.
If you are looking for a cache prefix, then see CACHE_PREFIX_* below

priyajeet · 2018-03-14T21:04:28Z

src/flowTypes.js

@@ -58,7 +59,7 @@ export type ClassComponent<P, S> = Class<React$Component<P, S>>;
 export type StringMap = { [string]: string };
 export type StringAnyMap = { [string]: any };
 export type StringBooleanMap = { [string]: boolean };
-export type ItemAPI = FolderAPI | FileAPI | WebLinkAPI;
+export type ItemAPI = FolderAPI | FileAPI | WebLinkAPI | VersionsAPI;


VersionsAPI should be separate from ItemAPI

priyajeet · 2018-03-14T21:05:49Z

src/api/APIFactory.js

@@ -145,6 +162,9 @@ class APIFactory {
            case TYPE_WEBLINK:
                api = this.getWebLinkAPI();
                break;
+            case TYPE_VERSIONS:


Should not be here, as thats for item type only

priyajeet · 2018-03-14T21:07:00Z

src/api/Item.js

+     * making file based XHRs where auth token
+     * can be per file as used by Preview.
+     *
+     * @return {string} typed id for file


This should be in File.js as before

priyajeet · 2018-03-14T21:09:38Z

src/api/Versions.js

+        // We only need the total_count for now
+        return this.xhr
+            .get({
+                id: this.getTypedFileId(id),


Should be referenced from FileAPI.
Can maybe make it a static function and update other places calling it.

priyajeet · 2018-03-14T21:10:53Z

src/components/ContentSidebar/ContentSidebar.js

@@ -43,11 +43,13 @@ type Props = {
    sharedLinkPassword?: string,
    requestInterceptor?: Function,
    responseInterceptor?: Function,
-    onInteraction: Function
+    onInteraction: Function,
+    onVersionHistoryClick?: Function


hasVersions: boolean needs to be added like other has* above

priyajeet · 2018-03-14T21:12:28Z

src/components/ContentSidebar/ContentSidebar.js

@@ -324,9 +352,10 @@ class ContentSidebar extends PureComponent<Props, State> {
            <Internationalize language={language} messages={messages}>
                <div id={this.id} className={`be bcs ${className}`}>
                    <div className='be-app-element'>
-                        {file ? (
+                        {file && versions ? (


Check should ideally not needed here, sidebar should render before versions is got

priyajeet · 2018-03-14T21:13:04Z

src/components/ContentSidebar/ContentSidebar.js

                            <Sidebar
                                file={file}
+                                versions={versions}


pass in hasVersions too

priyajeet · 2018-03-14T21:14:39Z

src/components/ContentSidebar/DetailsSidebar.js

@@ -56,6 +61,12 @@ const DetailsSidebar = ({

    return (
        <SidebarContent hasTitle={hasTitle} title={<FormattedMessage {...messages.sidebarDetailsTitle} />}>
+            {versions &&


Should be inside its own file, say SidebarVersions and here it will be
{hasVersions && <SidebarVersions versions={versions} onVersionHistoryClick={onVersionHistoryClick}>}

priyajeet · 2018-03-15T20:31:01Z

src/api/APIFactory.js

+    /**
+     * API for versions
+     */
+    getVersionsAPI(): VersionsAPI {


We may have to extend/improve these get*API calls to optionally take a flag that prevents calling this.destroy(). Until now it wasn't really needed because most API calls were not really called in parallel. Called one at a time. But versions will be called in parallel to say the File api call. So calling destroy will destroy the other. Likewise I didn't want two API calls to muck with the same cached object, but in versions case its a diff cache object, so not a big deal from that perspective.

Either that, or the call to getVersionAPI() should come in the fetch success callback of the file, so that its a serial call and not parallel.

priyajeet · 2018-03-15T20:34:15Z

src/components/ContentSidebar/ContentSidebar.js

        this.rootElement = ((document.getElementById(this.id): any): HTMLElement);
        this.appElement = ((this.rootElement.firstElementChild: any): HTMLElement);

        if (fileId) {
            this.fetchFile(fileId);
+            if (hasVersions) {


componentWillReceiveProps also has fetchFile(), this will likely need to be replicated there.
OR maybe have this.fetchVersions() inside fetchFile()

Refactoring componentWillReceiveProps a bit

priyajeet · 2018-03-15T20:35:38Z

src/components/ContentSidebar/DetailsSidebar.js

@@ -56,6 +63,7 @@ const DetailsSidebar = ({

    return (
        <SidebarContent hasTitle={hasTitle} title={<FormattedMessage {...messages.sidebarDetailsTitle} />}>
+            {hasVersions && <SidebarVersions onClick={onVersionHistoryClick} versions={versions} />}


onClick is fine for now, but may need a more descriptive name in the future if/when we pull in version switching and other on* actions.

priyajeet · 2018-03-15T20:37:36Z

src/components/ContentSidebar/SidebarVersions.js

+    }
+
+    return (
+        <section className='bcs-section'>


The other commit from jeremy has some improvements of <SidebarSection> without need of a title, perhaps see if that works well here.

priyajeet · 2018-03-15T20:39:01Z

src/components/ContentSidebar/SidebarVersions.js

+    );
+};
+
+SidebarVersions.defaultProps = {


Not the pattern we use, causes side effects for uglifyjs cleanup.
Assign the defaults within the props.

{ onVersionHistoryClick, versions = { total_count: 0 } }

Also will total count not be there?

total_count would not be there if API hasn't responded by time the component has rendered

priyajeet · 2018-03-15T20:40:09Z

src/flowTypes.js

@@ -338,3 +339,19 @@ export type MultiputPart = {
 export type MultiputData = {
    part?: MultiputPart
 };
+
+export type Version = {


I would refactor and make them the same, you would just need to add ? to properties that are optional and not needed for the object to be partial and just null check them. Should ideally not have two definitions of versions.

wenboyu2 · 2018-03-19T07:27:04Z

Webpack Bundle Size Github Bot

Bundle	Diff	Before	After
sidebar	3 KB	477 KB	474 KB
tree	1 KB	497 KB	496 KB
explorer	3 KB	674 KB	671 KB
uploader	1 KB	501 KB	500 KB
picker	1 KB	599 KB	598 KB
preview	3 KB	537 KB	534 KB
Total	12 KB	3.21 MB	3.2 MB

Compared against previously merged PR #206

New: version history in details tab

3369541

Chore: update comments

af475b6

priyajeet suggested changes Mar 14, 2018

View reviewed changes

DanDeMicco added 3 commits March 14, 2018 18:11

Chore: CR feedback

6a6484e

Merge remote-tracking branch 'upstream/master' into version-history

b8ac724

Chore: update snapshot

e21e0ce

priyajeet suggested changes Mar 15, 2018

View reviewed changes

DanDeMicco added 3 commits March 15, 2018 14:22

Merge remote-tracking branch 'upstream/master' into version-history

c84bb32

Chore: CR feedback

e42d966

Chore: update tests

7967df2

DanDeMicco mentioned this pull request Mar 16, 2018

New: Access stats component #227

Merged

priyajeet approved these changes Mar 19, 2018

View reviewed changes

DanDeMicco merged commit 935f617 into box:master Mar 20, 2018

DanDeMicco deleted the version-history branch March 20, 2018 19:13

sghosh326 mentioned this pull request Jun 13, 2018

Support Upload New Version in Content Explorer #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: version history in details tab #220

New: version history in details tab #220

DanDeMicco commented Mar 14, 2018 •

edited

Loading

boxcla commented Mar 14, 2018

priyajeet Mar 14, 2018

DanDeMicco Mar 14, 2018

priyajeet Mar 15, 2018

DanDeMicco Mar 15, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 14, 2018

priyajeet Mar 15, 2018

priyajeet Mar 15, 2018

DanDeMicco Mar 15, 2018

priyajeet Mar 15, 2018

priyajeet Mar 15, 2018 •

edited

Loading

DanDeMicco Mar 15, 2018

priyajeet Mar 15, 2018

DanDeMicco Mar 15, 2018

priyajeet Mar 15, 2018

wenboyu2 commented Mar 19, 2018

New: version history in details tab #220

New: version history in details tab #220

Conversation

DanDeMicco commented Mar 14, 2018 • edited Loading

boxcla commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenboyu2 commented Mar 19, 2018

Webpack Bundle Size Github Bot

DanDeMicco commented Mar 14, 2018 •

edited

Loading

priyajeet Mar 15, 2018 •

edited

Loading