Add basic shell for Vault view #23

DavidCorrea · 2023-04-07T09:08:52Z

Reviewer Details

This PR addresses the basic shell for where the File Tree and the folder content will be displayed.

Items Addressed

Basic Shell

Out of scope

Any navigational functionality
Selecting a file within the folder
Fetching files from back end (just display hard-coded file metadata for now)

Code Review Questions

On my understanding, the ticket was referring to having the two containers where all the information will be displayed. How it would be displayed sounded like it was going to be addressed in other ticket. I might have understood wrong, will implement missing part when I come back of OOO if so.
Please see my own comments in the code as I have questions there too.

Author Checklist

Review the diff and ensure all changes are relevant, intentional, and address all in-scope requirements
Assign a dev reviewer when you're ready for code review

DavidCorrea · 2023-04-07T09:09:38Z

fe/src/js/main/Layout.js

-      <Header />
-      <main id="main-content">{children}</main>
-      <Footer />


❓ - Do we want to keep the Header and Footer while they're not implemented yet?

Yeah I vote for removing the header and footer until we have them designed and implemented!

DavidCorrea · 2023-04-07T09:10:33Z

fe/src/js/main/Layout.js

@@ -22,13 +22,13 @@ function Layout({ flashMessages, pathname, children }) {
    scrollToTop()
  }, [pathname])
  return (
-    <div>
+    <div style={{ height: '100%' }}>


⚠️ - Totally dislike inlining styles, but I'm not sure how we want to approach having the app being full height. Do we use any library for that in other projects or do we do it on our own?

@DavidCorrea will this still be needed when the header and footer are eventually designed and added back in?

I'm a little ignorant on CSS practices...can we keep this 100% height but move it to the stylesheet?

DavidCorrea · 2023-04-07T09:11:15Z

fe/src/js/main/Layout.js

-      <Header />
-      <main id="main-content">{children}</main>
-      <Footer />
+      <main id="main-content" style={{ height: '100%' }}>


⚠️ - See comment in this line.

DavidCorrea · 2023-04-07T09:12:04Z

fe/src/js/main/vault/components/FileTree.js

+
+function FileTree({ folders }) {
+  return (
+    <aside className="vault__file-tree">


❓ - How do we want to approach CSS classes names? Do we want to use BEM? Any other convention?

If you are used to using BEM, I'd be good with that.

I'd defer to @ambernardi on this. Analisa, do you have any preferences on class naming conventions?

I think the two of you will be handling most of the styling, so I'm good with whatever y'all choose!

At the risk of proposing an additional technology, CSS Modules provides unique classnames without the developer-knowledge-overhead and eventually frequent structural updates required of BEM classes. Just an idea; not a hill I would die on.

I should note that styled-components also provides the same benefits listed above, but it's incredibly slow (it relies on extensive amounts of JS at runtime) and I've seen it abused in two different companies, so I can't recommend it.

DavidCorrea · 2023-04-07T09:12:34Z

fe/src/js/main/vault/components/FolderView.js

+import exact from 'prop-types-exact'
+
+const propTypes = {
+  content: PropTypes.array.isRequired,


💭 - Used content here, but mostly as a placeholder, it could be renamed to something more appropiate.

This is a tough one to name...

Maybe "items"? Still generic, but "content" makes me think it would be full of text.

I like items! Wanted something that could represent both files and folders but couldn't come up with anything. Items is perfect.

DavidCorrea · 2023-04-07T09:13:01Z

fe/src/js/main/vault/views/Vault.js

+  const fileTree = {
+    repository: 'Vault',
+    folders: [
+      {
+        id: 1,
+        parentFolderId: null,
+        createdAt: '2023-03-11 20:49:53.911774',
+        updatedAt: '2023-03-19 20:49:53.911774',
+        repository: 'Vault',
+        name: 'Northeast Vault',
+        childrenCount: 3,
+        path: '/Vault/Northeast Vault',
+      },
+      {
+        id: 2,
+        parentFolderId: null,
+        createdAt: '2023-03-11 20:49:53.911774',
+        updatedAt: '2023-03-19 20:49:53.911774',
+        repository: 'Vault',
+        name: 'West Coast Vault',
+        childrenCount: 0,
+        path: '/Vault/West Coast Vault',
+      },
+    ],
+    files: [],
+  }


💭 - As mentioned in the ticket, we're hardcoding this for now.

DavidCorrea · 2023-04-07T09:13:11Z

fe/src/scss/application.scss

+html,
+body,
+#root {
+  height: 100vh;
+  margin: 0;
+}


⚠️ - See comment in this line.

nicoledow · 2023-04-07T21:56:01Z

fe/src/js/main/Layout.js

-      <Header />
-      <main id="main-content">{children}</main>
-      <Footer />


Yeah I vote for removing the header and footer until we have them designed and implemented!

nicoledow · 2023-04-07T21:58:51Z

fe/src/js/main/vault/components/FolderView.js

+import exact from 'prop-types-exact'
+
+const propTypes = {
+  content: PropTypes.array.isRequired,


This is a tough one to name...

Maybe "items"? Still generic, but "content" makes me think it would be full of text.

nicoledow · 2023-04-08T01:43:47Z

fe/src/js/main/vault/components/FolderView.js

+
+const defaultProps = {}
+
+function FolderView({ content }) {


Maybe a name like "FolderContents" might be more appropriate for this component? "FolderView" makes me think it's a view 🤔

nicoledow · 2023-04-08T02:32:20Z

fe/src/js/main/Layout.js

@@ -22,13 +22,13 @@ function Layout({ flashMessages, pathname, children }) {
    scrollToTop()
  }, [pathname])
  return (
-    <div>
+    <div style={{ height: '100%' }}>


@DavidCorrea will this still be needed when the header and footer are eventually designed and added back in?

I'm a little ignorant on CSS practices...can we keep this 100% height but move it to the stylesheet?

nicoledow · 2023-04-08T02:34:01Z

fe/src/js/main/vault/components/FileTree.js

+
+function FileTree({ folders }) {
+  return (
+    <aside className="vault__file-tree">


If you are used to using BEM, I'd be good with that.

I'd defer to @ambernardi on this. Analisa, do you have any preferences on class naming conventions?

I think the two of you will be handling most of the styling, so I'm good with whatever y'all choose!

nicoledow · 2023-04-08T02:35:01Z

fe/src/js/main/vault/views/Vault.js

+    repository: 'Vault',
+    folders: [
+      {
+        id: 1,


I'll update this in the docs (sorry!), but the id fields will now be uuid strings instead of integers.

nicoledow · 2023-04-08T02:35:47Z

fe/src/js/main/vault/components/FileTree.js

+import exact from 'prop-types-exact'
+
+const propTypes = {
+  folders: PropTypes.array.isRequired,


can you add the prop shape to types.json and make this PropTypes.arrayOf(Types.folder)?

nicoledow · 2023-04-08T02:37:18Z

fe/src/js/main/vault/components/FolderView.js

+import exact from 'prop-types-exact'
+
+const propTypes = {
+  content: PropTypes.array.isRequired,


Same comment here as above, can we add a file shape to types.json and make this line PropTypes.arrayOf(Types.file).isRequired?

vanderhoop · 2023-04-10T16:47:22Z

fe/src/js/main/vault/components/FolderView.js

+  return (
+    <div className="vault__folder-view">
+      {content.map((folderOrFile, index) => {
+        return <div key={index}>{folderOrFile}</div>


key should ideally be an identifying attribute that's truly unique, as using an array item's index can cause bugs if the sort order changes.

https://react.dev/learn/tutorial-tic-tac-toe#picking-a-key

nicoledow

Looking awesome! Just wanted to rename a couple things after we learned some details about the Vault/ExhibitShare being combined in one toggle-able view. After we rename those I think we'll be good to go!

nicoledow · 2023-04-11T18:04:55Z

fe/src/js/main/vault/views/Vault.js

+
+const defaultProps = {}
+
+function Vault() {


After talking to Analisa about design plans, it sounds like Vault and ExhibitShare will be one page on which you can toggle between views.

Knowing that, maybe we rename this view? Maybe Library.js?

nicoledow · 2023-04-11T18:05:54Z

fe/src/js/main/Routes.js

@@ -17,6 +18,9 @@ function Routes() {
        <Route path="/styleguide">
          <StyleguideRoutes />
        </Route>
+        <Route path="/vault">


Similar to other comment, maybe we make this /library and rename the routes LibraryRoutes?

vanderhoop · 2023-04-11T18:43:10Z

fe/src/js/main/vault/components/FolderContent.js

+  items: PropTypes.oneOfType([
+    PropTypes.arrayOf(Types.folder).isRequired,
+    PropTypes.arrayOf(Types.file).isRequired,
+  ]),


Small refactor possibility would be to extract an Types.item type, which would be either a Types.folder or Types.file, as I'd imagine this union entity will appear in other places in the front-end app over time.

vanderhoop · 2023-04-11T18:48:39Z

fe/src/scss/application.scss

 // Styles for Assistive Technologies (keep these last!)
 @import 'components/visually-hidden';


@DavidCorrea I think this line and its associated comment should be kept at the very bottom of the file to avoid being inadvertently overwritten as this file grows.

nicoledow

One small request to update an key, but looks good to go! Nice work!

nicoledow · 2023-04-13T18:05:36Z

fe/src/js/main/library/components/FileTree.js

+    <aside className="library__file-tree">
+      {folders.map((folder, index) => {
+        return (
+          <div className="folder" key={index}>


can we update the key here to be folder.id?

DavidCorrea self-assigned this Apr 7, 2023

DavidCorrea commented Apr 7, 2023

View reviewed changes

nicoledow self-requested a review April 7, 2023 21:52

nicoledow requested changes Apr 8, 2023

View reviewed changes

vanderhoop reviewed Apr 10, 2023

View reviewed changes

DavidCorrea force-pushed the vault-basic-shell branch from 2471f9e to 0d43b89 Compare April 11, 2023 16:23

DavidCorrea requested review from nicoledow and vanderhoop April 11, 2023 16:23

nicoledow requested changes Apr 11, 2023

View reviewed changes

vanderhoop reviewed Apr 11, 2023

View reviewed changes

DavidCorrea requested review from nicoledow and vanderhoop April 13, 2023 12:21

DavidCorrea added 8 commits April 13, 2023 13:05

Add basic shell for Vault view

e0eba92

Use folder or file's ID for FolderView

4710c3a

Add Types for Files and Folders

803ed6e

Rename FolderView to FolderContent

e3a22d7

Adjust height styling

8a8d408

Adjust Vault index.js

fd96721

Add 'item' type

568a2c9

Rename Vault component to Library

a034873

DavidCorrea force-pushed the vault-basic-shell branch from a19830c to a034873 Compare April 13, 2023 16:05

nicoledow approved these changes Apr 13, 2023

View reviewed changes

nicoledow merged commit 4086b2c into main Apr 13, 2023

nicoledow deleted the vault-basic-shell branch April 13, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic shell for Vault view #23

Add basic shell for Vault view #23

DavidCorrea commented Apr 7, 2023

DavidCorrea Apr 7, 2023

nicoledow Apr 7, 2023

DavidCorrea Apr 7, 2023

nicoledow Apr 8, 2023

DavidCorrea Apr 7, 2023

DavidCorrea Apr 7, 2023

nicoledow Apr 8, 2023

vanderhoop Apr 11, 2023

vanderhoop Apr 11, 2023

DavidCorrea Apr 7, 2023

nicoledow Apr 7, 2023

DavidCorrea Apr 10, 2023

DavidCorrea Apr 7, 2023

DavidCorrea Apr 7, 2023

nicoledow Apr 7, 2023

nicoledow Apr 7, 2023

nicoledow Apr 8, 2023

nicoledow Apr 8, 2023

nicoledow Apr 8, 2023

nicoledow Apr 8, 2023

nicoledow Apr 8, 2023

nicoledow Apr 8, 2023

vanderhoop Apr 10, 2023

nicoledow left a comment

nicoledow Apr 11, 2023

nicoledow Apr 11, 2023

vanderhoop Apr 11, 2023

vanderhoop Apr 11, 2023

nicoledow left a comment

nicoledow Apr 13, 2023

		// Styles for Assistive Technologies (keep these last!)
		@import 'components/visually-hidden';

Add basic shell for Vault view #23

Add basic shell for Vault view #23

Conversation

DavidCorrea commented Apr 7, 2023

Reviewer Details

Items Addressed

Out of scope

Code Review Questions

Author Checklist

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

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

nicoledow left a 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

nicoledow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment