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

Add Capture button to IDE toolbar to update main image of Part #209

Merged
merged 6 commits into from
Feb 27, 2021
Merged

Add Capture button to IDE toolbar to update main image of Part #209

merged 6 commits into from
Feb 27, 2021

Conversation

franknoirot
Copy link
Collaborator

Adds a button to the CascadeStudio IDE to capture the camera view, then allows user to download the screenshot and to update the Part's main image. Related to #111 and #208. Designs can be seen in Figma. Happy to make any adjustments or edits needed!

Possible improvement could be to set up automatically saving a screenshot on first save of a Part, so all projects have a screenshot.

@Irev-Dev Irev-Dev self-requested a review February 25, 2021 10:43
@franknoirot
Copy link
Collaborator Author

Needs reworking to have setting the Part's image as the primary action, and to have tests written. Planning to work tonight or tomorrow (US Eastern).

@franknoirot
Copy link
Collaborator Author

@Irev-Dev I've updated so that the toolbar has a "Set Part Image" button instead of "Capture" per Figma feedback. This is ready for your review.

Pressing the button uploads and immediately overwrites whatever mainImage may have been set for the part, and then gives the user the opportunity to save a copy to their computer and shows them the image capture. This leads me to 2 questions:

  1. Is this too immediate an update? I'm wondering if future users will not like this if they happened to have uploaded an actual image to their part, then accidentally click this button and overwrite their upload. I guess that would be pretty rare though?
  2. Is there an expiration system set up in Cloudinary to delete orphaned images? This opens up the possibility of blowing through your free tier if the image storage adds up quickly.

Concerns

  • I added my capture() method to your cascadeController class, but I don't know if that was the right place to put it. My thought was it is in a sense controlling CascadeStudio?
  • I added another piece of useState to IdeToolbar.js. Was that okay? Is there enough state in there that it should be transitioned to use useReducer?
  • There is a bit of a layout bug that occurs on capture: the alert banner that shows the part has been saved causes the popup dialog to jump down (see attached images). Could this be fixed by using position: fixed to style this popup, so it isn't affected by document flow?

Future Work

I was unable to figure out how to automatically capture a screenshot on initial save of a Part: I need a reference to the part to complete my onCapture() call, but after initial save navigate() is called and appears to refresh the page and wipe the current state. Would love any help on this.

@Irev-Dev
Copy link
Owner

This looks really good @franknoirot 🙌

You've done a good job of figuring out the structure i.e. putting the svgs with the rest of them. I like the cloundinary helper and I think putting the capture on the cascade studio controller is the perfect place for that, and it all done in only 160 odd lines of code.

Questions

  1. I think you might be right, it does seem very sudden, I guess I chose poorly form your options on figma and the original option of having a popup that has "download" and "set part image" as options would be better? I think maybe keeping the copy for the tool bar button as set part image is still a good idea just to highlight that usecase? (also maybe lets change the work set to save or create. I think set is a little to programmer talk. What do you think?

  2. Yes the images should be destroyed, when the part is updated if the image id changes it tells Cloudinary to remove the old image. see https://github.com/Irev-Dev/cadhub/blob/main/api/src/services/parts/parts.js#L79
    It possible you see an error in your terminal locally though since uploading doesn't require having the API secret but destroying does and you wouldn't have that. I just sent you details on discord.

Concerns

  • Capture on the controller looks good to me.

  • Extra state is fine too. There's no global state management tool in the app at the moment (no redux or similar or a global context and reducer) That's partly because in some ways apollo can kinda fill that gap, since it caches queries it's fine to just call fire those queries often, (you can even use apollo to do local state management, they have these local/client mutations etc that you can do, I'm not completely sold on it though). Plus everything part of state so far is either something that should be put in the db (so apollo kinda handles it) or it specific to a component so doesn't need full-blown state management. It's likely at some point that we might want/need proper state management, but I don't think it's something worth implementing when it's not needed yet, plus state management is one of the few things that RedWood doesn't have a strong opinion on, and I half expected that to change. If they come out with an officially recommended way of doing it I'm inclined to follow them from the perspective of keeping it as standard as possible as a way of making it easier for others to contribute. Sorry that was a long way of saying that adding the extra useState is 100% fine. If the toolbar starts getting too crowded we can make each button it's own small component instead, that's probably more the direction I'd go.

  • That's a good point. That flash is just the standard Redwood one, and this page is not the only place that it pushing down content is a little jaring, a quick fix, like you said position: fixed will work. I just added fixed w-screen z-10 to this line. Maybe at some point we'll change it to a more custom toast message. (I've had my eye on https://react-hot-toast.com/ for a while)

changes

That's a lot of words, here's what I think should change.

  • If your happy to go back to your original idea of giving two options in the popup, that might be better.
    image

That's all really it, you can tweak the flash if you want.

I'll have a bit of a look into capture on first save. 🤞

@franknoirot
Copy link
Collaborator Author

Woot thanks! ✨

Questions

  1. I don't mind turning it back! It's two interactions but in some cases I think it's worth it to get confirmation. I agree with "Save Part Image", that is better than "Set".
  2. Alright great 👌🏻

Concerns

  • Great, yeah maybe in little I can do that component breakout in here. It's not too bad right now I just realized my little button came with a fair amount of business logic. I bet that bears out with Redwood, since they're really staking out that "opinionated framework" brand. Oo yeah using a db client to do local state management makes me feel weird for some reason, but I'll check that out.
  • Ah sweet I'll update with your quick fix now, and maybe you can roadmap react-hot-toast, it looks crispy.

Will comment in after I push the update.

@Irev-Dev
Copy link
Owner

I got capture on first save working.
104bc0a

Happy for you to cherry-pick that commit or I can put up a PR after this one.

Note that there are some formatting changes in that commit, the project automatically formats on save for me, it's not something I set up I just think that's how redwood is setup to work when it used in vscode, I was going to ask you to run yarn rw lint to format your branch because I thought that was a way to get it to format without the auto-save, but I get an error when I do that 🤷 . I'm a couple of version behind latest-redwood so I should probably update it maybe that will fix that command. Anyway, formatting is not a big problem.

@franknoirot
Copy link
Collaborator Author

franknoirot commented Feb 27, 2021

Oh sick @Irev-Dev way to go! Do you mind doing that in a different PR? I don't feel confident with Git yet to do a cherry-pick like that, you'll probably be way faster at that.

My stuff is ready for your review when you're free. I made it so that if your part happens to not have a screenshot yet when you click Save Part Image button it saves right away without needing to click the second thing as well.

Copy link
Owner

@Irev-Dev Irev-Dev left a comment

Choose a reason for hiding this comment

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

Thanks again @franknoirot.
The app is definitely a lot better because of this.

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.

None yet

2 participants