Flash messages layout #38
Conversation
| <span><%= msg %></span> | ||
| </div> | ||
| <% end %> | ||
| <div id='flash-container'> |
There was a problem hiding this comment.
| <div id='flash-container'> | |
| <div class='flash-container'> |
should rather be a class
| background-color: rgb(from var(--error-color) r g b / 80%); | ||
| } | ||
|
|
||
| button.close { |
There was a problem hiding this comment.
| button.close { | |
| .btn-close { |
I think I'd prefer strict usage of css classes
| @@ -84,3 +91,58 @@ table { | |||
| width: 100%; | |||
| } | |||
| } | |||
|
|
|||
| #flash-container { | |||
| position: fixed; | |||
There was a problem hiding this comment.
why do we use a fixed position? I think this make the flash message still appear "above" the model heading
There was a problem hiding this comment.
yes it does slightly, but has some opacity so you can still read the heading, and adding too much margin-top on the heading to prevent this would look a bit off as well.
On the other hand, having it inline would cause all the context to be pushed down, which is confusing and annoying.
So I personally like how it looks now, but I can think of alternatively moving the flash to the bottom-right of the screen like this:
There was a problem hiding this comment.
Yes, good idea! I would add it at the bottom.
Still, I would be annoyed by the "updated sucessfully" flash msgs. Imagine you would have this in a google spreadsheet? If you update multiple rows, each returning a flash msg?
| flash[:notice] = t("hotsheet.success", record: model.model_name.human) | ||
| flash[:success] = t("hotsheet.success", record: model.model_name.human) |
There was a problem hiding this comment.
should we really keep the flash msg for success state? I don't think we should display it in this case
There was a problem hiding this comment.
you mean show no flash at all on success and only on failure? I find important having some visual feedback that your action was performed
There was a problem hiding this comment.
I replied in the chat above regarding this
ignaciosy
force-pushed
the
feature/flash-layout
branch
from
1247b96 to
df62803
Compare
| @@ -84,3 +91,58 @@ table { | |||
| width: 100%; | |||
| } | |||
| } | |||
|
|
|||
| #flash-container { | |||
| @@ -84,3 +91,58 @@ table { | |||
| width: 100%; | |||
| } | |||
| } | |||
|
|
|||
| #flash-container { | |||
| position: fixed; | |||
There was a problem hiding this comment.
Yes, good idea! I would add it at the bottom.
Still, I would be annoyed by the "updated sucessfully" flash msgs. Imagine you would have this in a google spreadsheet? If you update multiple rows, each returning a flash msg?
|
|
||
|
|
| background-color: rgb(from var(--error-color) r g b / 80%); | ||
| } | ||
|
|
||
| button.close { |
| flash[:notice] = t("hotsheet.success", record: model.model_name.human) | ||
| flash[:success] = t("hotsheet.success", record: model.model_name.human) |
There was a problem hiding this comment.
I replied in the chat above regarding this
lib/hotsheet/version.rb
Outdated
| VERSION = "0.1.1" | ||
| VERSION = "0.1.2" |
There was a problem hiding this comment.
let's bump the version only on the main branch. not every PR/feature needs a separate release
Closes #29 and is related to #15
Screen.Recording.2025-01-15.at.14.46.05.mov
Also defined some base css variables like colors, so we can adjust them easily in the future.