Skip to content

Feature/dsm 169 - reorganize rails routes#70

Merged
goldsmithb merged 4 commits into
s3-browserfrom
feature/DSM-169
Jun 2, 2026
Merged

Feature/dsm 169 - reorganize rails routes#70
goldsmithb merged 4 commits into
s3-browserfrom
feature/DSM-169

Conversation

@goldsmithb

Copy link
Copy Markdown

Jira Ticket


This branch renders the React SPA root element (and application) at the root route of the rails application.

If a user is unauthenticated, they are immediately redirected to the sign in view.

To enable people to sign out of the app, I also added a bar at the top of the s3 app layout with the sign out link button.

And to make sure any error toasts don't overlap the sign out button, I adjusted the style of the Notifications component.

image

@goldsmithb goldsmithb requested a review from wtomaszewska June 2, 2026 14:32
Comment thread app/views/layouts/s3_browser.html.erb Outdated
<% # The sign out route uses a DELETE method to prevent CSRF attacks. %>
<p>
<%= form_with(url: destroy_user_session_path, method: "delete") do |form| %>
<%= form.submit "Sign out" %>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would be nice to have this element use Bootstrap to match the rest of the app, for example:

Suggested change
<%= form.submit "Sign out" %>
<%= form.submit "Sign out", class: "btn btn-light btn-sm" %>

Comment thread app/views/layouts/s3_browser.html.erb Outdated

<%= yield :head %>
</head>
<nav class="d-flex py-2 px-5 bg-primary bg-opacity-25 justify-content-end">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think this is valid HTML (nav has to be a child of body), but somehow the DOM renders it correctly as 'body > nav'.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was curious and did some googling, and it seems the browser is inferring where it belongs -- apparently some tags are technically optional and browsers can infer where content goes in the DOM! Didn't know that was a thing

I'll move it inside body regardless

@wtomaszewska wtomaszewska left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks great! Left just a minor suggestion if we want to use Rails to render UI elements.

@goldsmithb goldsmithb merged commit d8a135c into s3-browser Jun 2, 2026
1 check passed
@goldsmithb goldsmithb changed the title Feature/dsm 169 Feature/dsm 169 - reorganize rails routes Jun 2, 2026
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