Closed raghubetina closed 4 years ago
@pmckernin @jelaniwoods The easiest way to review this is probably to generate a resource with the original branch, and then generate the same resource with the new branch, and then look at the diff.
@raghubetina Made some comments here: https://github.com/jelaniwoods/wip_app/pull/1
@raghubetina I made some minor updates to fix formatting issues. There are a couple of comments I made before that I wasn't sure how to handle yet.
@jelaniwoods draft:layout
needs a lot of love.
javascript_include_tag
changing to javascript_pack_tag
nowadays.See this commit for a reworked version:
The navbar resource links generator smarts still haven't been updated:
@jelaniwoods Looks great. Most of my review is here:
https://github.com/raghubetina/test-golden-5-resources/pull/1
@jelaniwoods
draft:layout
needs a lot of love.
- At present it breaks due to
javascript_include_tag
changing tojavascript_pack_tag
nowadays.- Assets versions are outdated.
- CDNs need to be updated.
See this commit for a reworked version:
I think that should be a separate pr. I will get started on that.
@raghubetina, I made the changes you suggested
@jelaniwoods Two things re: draft:resource
:
I think we should consider demonstrating how to display validation failure messages, since they are so important. Here is how I might consider generating code for that:
https://github.com/raghubetina/test-golden-5-resources/pull/3
Thoughts? If we don't do that, then I would consider redirecting in the update
action also to be consistent with what is being done in the create
action. Which way do you think we should go?
Thoughts? If we don't do that, then I would consider redirecting in the
update
action also to be consistent with what is being done in thecreate
action. Which way do you think we should go?
@raghubetina do you mean that both create
and update
actions should redirect to the resource show page?
@raghubetina do you mean that both create and update actions should redirect to the resource show page?
No, I mean if we don't render
views with validation failure messages (which is fine for this PR, we can do it in a separate PR tomorrow), then we should probably redirect_to
the show page with a notice from the update action when the save doesn't work, the way that the create action is currently doing to the index page (for consistency).
Thoughts? If we don't do that, then I would consider redirecting in the
update
action also to be consistent with what is being done in thecreate
action. Which way do you think we should go?@raghubetina do you mean that both
create
andupdate
actions should redirect to the resource show page?
I clarified with a suggested code change.
@raghubetina I've added the spaces around the :notice
s and updated the update
action to redirect
with an alert
message.
@jelaniwoods LGTM. Once you and @pmckernin give it a final whirl, go ahead and merge everything into summer-19
. Thanks!
@jelaniwoods Have these changes made it into the branch that students are using for their final projects (summer-2019
, I believe)?
Right now in the new forms, checkboxes don't work quite right and the indentation is wonky. This patch seeks to address both.