Skip to content

Create Asset List View and refactor overlay code #356

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

Merged
merged 9 commits into from
Jul 11, 2017
Merged

Conversation

catarak
Copy link
Member

@catarak catarak commented May 18, 2017

This is a fix for issue #169. This creates a view for all of a user's assets on S3, and refactors the overlay component code a bit. Some open questions I have:

  • Right now a user's assets are being stored on S3 in a folder that is their user id. Would it make sense to also store it in a folder that is the project id, i.e. <user_id>/<project_id>/<asset_name>? There's some discussion about this on Add view for all of a user's S3 files #169. I say this because matching assets to their respective projects is a little slow, but maybe that is just fine.
  • The overlay code is better but I think there's some redundancy in the redux state. I think we need one variable that contains whether or not an overlay is visible, and another variable (probably an enum, which in JS can just be a string) that is the contents of the overlay. This can probably be a separate PR.
  • Should users be able to delete assets from this view? This brings up the question, if a user deletes an asset from this view, does the asset get deleted from the sketch too? Would it instead make sense to restrict deletion to within a sketch only?

@catarak catarak requested a review from andrewn May 18, 2017 20:59
</tr>
</thead>
<tbody>
{this.props.assets.map(asset =>
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to show a short message when the user has no assets and when the asset list is being fetched? Maybe it's quick enough not to be an issue but initially I was looking at the Overlay wondering if I should be seeing anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's slow and it may be nice to have a loading state for this view, which could also be used for the SketchList view as well.

Also agree that this should handle the case in which there are no assets 😸

{this.props.assets.map(asset =>
<tr className="asset-table__row visibility-toggle" key={asset.key}>
<td className="asset-table__delete">
<button
Copy link
Member

Choose a reason for hiding this comment

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

This button is rendered in the DOM but I can't see it in the UI. It's also not clickable.

screenshot 2017-05-25 12 00 23

@@ -1,3 +1,5 @@
// TODO Organize this file by reducer type, ot break this apart into
// multiple files
Copy link
Member

Choose a reason for hiding this comment

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

👍 I think having different files would be good as I struggled a bit figuring out where new action constants should go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this file has gotten crazy, and can definitely be broken apart. Not sure if it would be better to put this in a folder called "constants" or maybe follow the "ducks" redux design pattern.

@andrewn
Copy link
Member

andrewn commented May 25, 2017

This looks really good! I made some small inline comments.

Overlay

I think the Overlay refactoring is great and cleans things up a lot. I agree about having an Overlay Enum. If it's set then we can display the enum.

Uploading same asset multiple times

I uploaded the same pic.jpg multiple times to the same sketch and it appeared 3 times in the project file browser. However, it is only listed once in the Asset List. I checked the S3 Developer Console and there are 3 assets listed there.

screenshot 2017-05-25 12 08 43

Asset listing being slow / storing by project

I say this because matching assets to their respective projects is a little slow, but maybe that is just fine.

Would this actually speed up the displaying of the list as we'd still need to fetch all the projects and assets to be displayed? This feels like something we could do later? I think that having some sort of loading spinner UI whilst the client is waiting for the data from the API might make it seem snappier?

Deleting assets from Asset List view

My expectation would be that if I delete an asset from this view then it would disappear from the project view too. It might be enough to allow the Asset List columns to be sortable so that users can find what they're looking for and then go straight to the project to manage the assets for that project?

@catarak
Copy link
Member Author

catarak commented May 25, 2017

I purposefully hid the delete icon as I wasn't sure if it made sense to add the ability to delete an asset here, but I think it doesn't make sense so I'll remove it.

I'm not sure why the image you uploaded multiple times isn't appearing. That seems like a bug 😈

@catarak catarak merged commit e140702 into master Jul 11, 2017
@catarak catarak deleted the asset-list branch July 11, 2017 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants