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 edit functionality for notes #101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AI-Mozi
Copy link
Member

@AI-Mozi AI-Mozi commented Apr 21, 2024

#58

@AI-Mozi AI-Mozi marked this pull request as draft April 21, 2024 19:14
@AI-Mozi
Copy link
Member Author

AI-Mozi commented Apr 21, 2024

@postmodern take a look at it please and let me know if you like it or what would you change. Code is not fully completed, I'll have to "refactor" it a bit later.

Copy link
Member

@postmodern postmodern left a comment

Choose a reason for hiding this comment

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

Needs HTML escaping in the views, especially on note.body; or we should render note.body as markdown. Also should add hattr to all emitted attributes in the views, "just to be safe". I'm strongly against pulling in any external JavaScript assets, especially fonts, and prefer we just use the user's browser's default font.

views/_notes.erb Outdated
<div class="control">
<div class="media-content">
<div class="field mb-0 p-2">
<textarea id="note-edit-textarea-<%= note.id %>" class="textarea" name="body" placeholder="Edit a note..."><%= note.body %></textarea>
Copy link
Member

Choose a reason for hiding this comment

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

Needs h HTML escaping, or we need to render the note body as markdown and inline the primitive HTML.

views/_notes.erb Outdated

<div class="field is-flex is-justify-content-flex-end px-2 pb-2">
<button class="edit-note button is-small is-danger mr-2" data-note-id=<%= note.id %>>Discard</button>
<button class="update-note button is-small is-primary" data-note-id=<%= note.id %> data-note-body=<%= note.body %>>Save</button>
Copy link
Member

Choose a reason for hiding this comment

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

Attributes need hattr escaping.

views/_notes.erb Outdated

<div class="columns is-full">
<div class="column">
<div id="note-edit-body-<%= note.id %>" class="mb-0 p-4"><%= note.body %></div>
Copy link
Member

Choose a reason for hiding this comment

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

Needs h HTML escaping on note.body, or we should render it as markdown.

views/_notes.erb Outdated
<div class="box p-0" style="border: 1px solid #1c1c39">
<div class="columns m-0 p-1" style="background-color: #1c1c39">
<div class="column is-half px-3 py-0">
<small class="is-size-7">Created at: <%= note.created_at %></small>
Copy link
Member

Choose a reason for hiding this comment

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

Probably safe, but I'd escape the timestamp using h just to be safe.

views/_notes.erb Outdated
<div class="dropdown-menu" id="dropdown-menu6" role="menu">
<div class="dropdown-content">
<a class="dropdown-item py-2 edit-note" data-note-id=<%= note.id %>>Edit</a>
<a class="dropdown-item py-2 delete-note has-text-danger" data-note-id=<%= note.id %>>Delete</a>
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to escape note.id with hattr just to be safe, and that it doesn't trigger false positives on some security scan.

views/_notes.erb Outdated

<div class="columns is-full">
<div class="column">
<div id="note-edit-body-<%= note.id %>" class="mb-0 p-4"><%= note.body %></div>
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to escape note.id with hattr just to be safe, and that it doesn't trigger false positives on some security scan.

views/_notes.erb Outdated
<div class="column">
<div id="note-edit-body-<%= note.id %>" class="mb-0 p-4"><%= note.body %></div>

<div id="note-edit-form-<%= note.id %>" style="display: none">
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to escape note.id with hattr just to be safe, and that it doesn't trigger false positives on some security scan.

views/_notes.erb Outdated
<div class="control">
<div class="media-content">
<div class="field mb-0 p-2">
<textarea id="note-edit-textarea-<%= note.id %>" class="textarea" name="body" placeholder="Edit a note..."><%= note.body %></textarea>
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to escape note.id with hattr just to be safe, and that it doesn't trigger false positives on some security scan.

views/layout.erb Outdated
@@ -8,6 +8,7 @@
<link rel="stylesheet" type="text/css" media="screen" href="/stylesheets/bulma.min.css" />
<link rel="stylesheet" type="text/css" media="screen" href="/stylesheets/app.css" />
<script type="text/javascript" src="/javascript/app.js"></script>
<script defer src="https://use.fontawesome.com/releases/v5.3.1/js/all.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add a font just yet and use the browser's default font, especially not some external JavaScript asset font. The whole app should be able to load and run without an internet connection.

app/db.rb Outdated
put "/db/notes/:id" do
@record = Ronin::DB::Note.find_by(params[:id])

unless @record || @record.update(params)
Copy link
Member

Choose a reason for hiding this comment

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

I would return a different error code if the update fails, like 400 (Bad Request).

@postmodern
Copy link
Member

Noticed that noteId is used a lot in multiple id elements. I wonder if we could use data attributes for that? Or have one query to find the outer most div with the noteId, and then sub-select the specific elements within the div based on class or element type.

@postmodern
Copy link
Member

postmodern commented Apr 22, 2024

Oooh the JavaScript asset is for the icons. For SVG icons just download the desired icon files and put them into public/images/. Previously I would just do a Google Images search for "SVG edit icon" and filter by Creative Commons. I recently discovered iconduck.com which is a Free and Open Source collection of icons, and offers SVG downloads. We should probably just use individual icons from them.

@postmodern
Copy link
Member

Doing a visual inspection, I think the grey text on black background for the comment header is hard to see in bright-mode. I'd use white text on a black background, or at least a brighter shade of grey.
localhost_1337_db_host_names_1 (1)

@postmodern
Copy link
Member

It also appears that the Delete doesn't remove the comment div immediately. Edit works perfectly thought.

@AI-Mozi AI-Mozi marked this pull request as ready for review April 22, 2024 08:50
@AI-Mozi AI-Mozi requested a review from postmodern April 22, 2024 08:50
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