gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Add new category flow #1857

Merged
syedbarimanjan merged 14 commits from Improvement/new-add-category-flow into main 2025-02-25 00:08:05 +08:00
Conversation 30 Commits 14 Files Changed 14 +335 -40
syedbarimanjan commented 2025-02-13 22:52:10 +08:00 (Migrated from github.com)
Copy Link

A video showing the changes in action:

https://github.com/user-attachments/assets/ee8b7dae-40f9-493a-9ca2-fb7dc9fb6e0a

Fixes #1734
/claim #1734

A video showing the changes in action: https://github.com/user-attachments/assets/ee8b7dae-40f9-493a-9ca2-fb7dc9fb6e0a Fixes #1734 /claim #1734
syedbarimanjan (Migrated from github.com) reviewed 2025-02-13 22:55:59 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-13 22:55:58 +08:00
Copy Link

This code has been taken from the webaim.orgs contrast checker.

This code has been taken from the webaim.orgs contrast checker.
syedbarimanjan commented 2025-02-13 23:14:29 +08:00 (Migrated from github.com)
Copy Link

@zachgoll can you please approve the workflow and review.

@zachgoll can you please approve the workflow and review.
zachgoll (Migrated from github.com) reviewed 2025-02-14 01:43:55 +08:00
app/assets/stylesheets/@simonwep/pickr/themes/monolith.min.css
zachgoll (Migrated from github.com) commented 2025-02-14 00:58:56 +08:00
Copy Link

I think we should update this to: app/assets/stylesheets/simonweb_pickr.css and then import it into our main application.css tailwind file:

@import "../stylesheets/simonweb_pickr.css"
I think we should update this to: `app/assets/stylesheets/simonweb_pickr.css` and then import it into our main `application.css` tailwind file: ``` @import "../stylesheets/simonweb_pickr.css" ```
app/assets/stylesheets/application.css
zachgoll (Migrated from github.com) commented 2025-02-14 01:01:54 +08:00
Copy Link

We've made some recent changes to the Tailwind files, so you'll want to pull those in first. After that, we should move this to the @layer components directive within the tailwind/application.css file.

We've made some recent changes to the Tailwind files, so you'll want to pull those in first. After that, we should move this to the `@layer components` directive within the `tailwind/application.css` file.
app/javascript/controllers/color_avatar_controller.js
zachgoll (Migrated from github.com) commented 2025-02-14 01:08:07 +08:00
Copy Link

Looks like something happened here with the parent change handling:

https://github.com/user-attachments/assets/263d2342-b67d-439e-a5c0-33330e862c4b

Looks like something happened here with the parent change handling: https://github.com/user-attachments/assets/263d2342-b67d-439e-a5c0-33330e862c4b
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-14 01:38:59 +08:00
Copy Link

I'm a little worried that we're doing too much here. I think if we choose to integrate a 3rd party picker library, we shouldn't need this amount of code to simply choose a color.

I think we need to either:

  1. Simplify this controller so that it initializes the color picker JS library and acts as a simple bridge between that library and our form (my guess is that the color library can tell us if the contrast is valid? Or we could at least write a simpler validation)
  2. Use the browser's built in <input type="color" /> (pictured below)

CleanShot 2025-02-13 at 12 34 55

cc @justinfar would you be okay with a basic, native color picker like I've shown above if it drastically reduces the complexity of this feature? (we can style the button to open it better than i did here—this was just for an example)

I'm a little worried that we're doing too much here. I think if we choose to integrate a 3rd party picker library, we shouldn't need this amount of code to simply choose a color. I think we need to either: 1. Simplify this controller so that it initializes the color picker JS library and acts as a simple bridge between that library and our form (my guess is that the color library can tell us if the contrast is valid? Or we could at least write a simpler validation) 2. Use the browser's built in `<input type="color" />` (pictured below) ![CleanShot 2025-02-13 at 12 34 55](https://github.com/user-attachments/assets/cc9444f1-8c58-4f89-8ac1-22959db23fd8) cc @justinfar would you be okay with a basic, native color picker like I've shown above if it drastically reduces the complexity of this feature? (we can style the button to open it better than i did here—this was just for an example)
syedbarimanjan (Migrated from github.com) reviewed 2025-02-14 02:41:42 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-14 02:39:08 +08:00
Copy Link

I'm a little worried that we're doing too much here. I think if we choose to integrate a 3rd party picker library, we shouldn't need this amount of code to simply choose a color.

All of the pure/vanilla ja libraries have been made with no customization in which is why we need this much code.

Simplify this controller so that it initializes the color picker JS library and acts as a simple bridge between that library and our form (my guess is that the color library can tell us if the contrast is valid? Or we could at least write a simpler validation)

The color library is only providing us the color preview box and the slider beneath it the validation logic is seperate.

Use the browser's built in (pictured below)

This will simplify the code alot but the picker element can not be changed in any way, cant even change where it will open.

Keep in mind that if we even use the built in color picker we would still need the code from line 50 till 119 to validate the contrast.

I would recommend to use the builtin color picker as the custom design isnt needed for this most of the users are used to the builtin input and it works fine.

> I'm a little worried that we're doing too much here. I think if we choose to integrate a 3rd party picker library, we shouldn't need this amount of code to simply choose a color. All of the pure/vanilla ja libraries have been made with no customization in which is why we need this much code. >Simplify this controller so that it initializes the color picker JS library and acts as a simple bridge between that library and our form (my guess is that the color library can tell us if the contrast is valid? Or we could at least write a simpler validation) The color library is only providing us the color preview box and the slider beneath it the validation logic is seperate. >Use the browser's built in <input type="color" /> (pictured below) This will simplify the code alot but the picker element can not be changed in any way, cant even change where it will open. Keep in mind that if we even use the built in color picker we would still need the code from line 50 till 119 to validate the contrast. I would recommend to use the builtin color picker as the custom design isnt needed for this most of the users are used to the builtin input and it works fine.
justinfar (Migrated from github.com) reviewed 2025-02-14 04:14:42 +08:00
app/javascript/controllers/color_picker_controller.js
justinfar (Migrated from github.com) commented 2025-02-14 04:14:42 +08:00
Copy Link

Would prefer to keep the color picker custom if possible, but if this is causing too much complexity I'm fine with the browser native color picker

Would prefer to keep the color picker custom if possible, but if this is causing too much complexity I'm fine with the browser native color picker
goldenstein64 commented 2025-02-14 06:51:13 +08:00 (Migrated from github.com)
Copy Link

Usability concern: Releasing the mouse from outside the modal always closes it, even when the mouse was pressed from inside the modal. Dragging motions to the outside feel common with this UI, so it would be annoying to close unintentionally, especially if there was more information written in the previous window.

https://github.com/user-attachments/assets/bfdae9a5-7f93-4faf-8e58-f5fd9c5cd71e

My thoughts are to make the modal view close iff the mouse was pressed and released from outside, something along the lines of a stopPropagation call / :stop action on the <dialog>'s only child and adding a flag to the modal controller that gets set on mouse down.

Usability concern: Releasing the mouse from outside the modal always closes it, even when the mouse was pressed from inside the modal. Dragging motions to the outside feel common with this UI, so it would be annoying to close unintentionally, especially if there was more information written in the previous window. https://github.com/user-attachments/assets/bfdae9a5-7f93-4faf-8e58-f5fd9c5cd71e My thoughts are to make the `modal` view close iff the mouse was pressed _and_ released from outside, something along the lines of a `stopPropagation` call / `:stop` action on the `<dialog>`'s only child and adding a flag to the modal controller that gets set on mouse down.
syedbarimanjan (Migrated from github.com) reviewed 2025-02-14 22:51:00 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-14 22:51:00 +08:00
Copy Link

@zachgoll your final comments?

@zachgoll your final comments?
zachgoll (Migrated from github.com) reviewed 2025-02-17 22:11:24 +08:00
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-17 22:11:24 +08:00
Copy Link

Let's go ahead with the following solution:

  • Keep the custom pickr library + Stimulus controller
  • Simplify the contrast checking to use WCAG 2.1 guideline for contrast (1.4.3), which simply checks relative luminance values.
    • The background should be the given color mixed with white at 10% opacity
    • Let's work exclusively with rgb/rgba values to avoid all the conversion functions
    • In total, we should be able to do this with just 3 functions: backgroundColor(rgba), luminance(rgb), and contrast(luminanceForeground, luminanceBackground)
  • When a color does not meet the contrast requirement of 4.5:1, the following should happen:
    • Keep the value in the input untouched
    • Put a red border around the input with a red, text-xs message above saying "Choose a darker color"
    • Use the setCustomValidity method to also add this same validation at the form level (i.e. "Color is too light") to ensure that if the user fails to update their selection, the form will not submit.
Let's go ahead with the following solution: - Keep the custom `pickr` library + Stimulus controller - Simplify the contrast checking to use [WCAG 2.1 guideline for contrast (1.4.3)](https://www.w3.org/TR/WCAG21/#contrast-minimum), which simply checks relative luminance values. - The background should be the given color mixed with white at 10% opacity - Let's work exclusively with rgb/rgba values to avoid all the conversion functions - In total, we should be able to do this with just 3 functions: `backgroundColor(rgba)`, `luminance(rgb)`, and `contrast(luminanceForeground, luminanceBackground)` - When a color does not meet the contrast requirement of `4.5:1`, the following should happen: - Keep the value in the input untouched - Put a red border around the input with a red, `text-xs` message above saying "Choose a darker color" - Use the [setCustomValidity](https://developer.mozilla.org/en-US/docs/Web/API/HTMLObjectElement/setCustomValidity) method to also add this same validation at the form level (i.e. "Color is too light") to ensure that if the user fails to update their selection, the form will not submit.
zachgoll commented 2025-02-17 22:13:11 +08:00 (Migrated from github.com)
Copy Link

@goldenstein64 would you mind opening a new issue for this? I agree with your suggestion, but since it applies to all of our modals / drawers in the app, it's probably worth a standalone issue.

@goldenstein64 would you mind opening a new issue for this? I agree with your suggestion, but since it applies to all of our modals / drawers in the app, it's probably worth a standalone issue.
👍 1
syedbarimanjan (Migrated from github.com) reviewed 2025-02-18 04:42:07 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-18 04:42:07 +08:00
Copy Link

@zachgoll we are getting the color in hex format from the user so we will have to convert it to rgb which will make the function count to 4.

@zachgoll we are getting the color in hex format from the user so we will have to convert it to rgb which will make the function count to 4.
zachgoll (Migrated from github.com) reviewed 2025-02-18 05:03:22 +08:00
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-18 05:03:22 +08:00
Copy Link

The picker.on('change') event should pass an HSVaColor Object, so I was thinking we can use toRGBA() for contrast checking, and call toHEXA() for the actual input value.

The `picker.on('change')` event should pass an [HSVaColor Object](https://github.com/simonwep/pickr?tab=readme-ov-file#the-hsvacolor-object), so I was thinking we can use `toRGBA()` for contrast checking, and call `toHEXA()` for the actual `input` value.
syedbarimanjan commented 2025-02-18 08:08:56 +08:00 (Migrated from github.com)
Copy Link

@zachgoll

https://github.com/user-attachments/assets/2b07b0b6-5e76-4d1f-b7a1-818f6d227c46

@zachgoll https://github.com/user-attachments/assets/2b07b0b6-5e76-4d1f-b7a1-818f6d227c46
zachgoll (Migrated from github.com) reviewed 2025-02-19 01:31:18 +08:00
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-18 20:24:49 +08:00
Copy Link
      const backgroundColor = this.backgroundColor(rgbacolor,10);
```suggestion const backgroundColor = this.backgroundColor(rgbacolor,10); ```
zachgoll (Migrated from github.com) commented 2025-02-19 01:29:59 +08:00
Copy Link

Contrast checking looks good!

Contrast checking looks good!
app/views/categories/_form.html.erb
@@ -1,42 +1,70 @@
<%# locals: (category:, categories:) %>
<div data-controller="color-avatar">
<div data-controller="category" data-category-preset-colors-value="<%= Category::COLORS %>">
zachgoll (Migrated from github.com) commented 2025-02-19 01:07:37 +08:00
Copy Link

There are a few style mismatches here. In order to clear out the default styled_form input styles, you can pass inline: true which tells our form builder to omit default styles:

<%= f.text_field :color , data: { color_picker_target: "colorInput", color_avatar_target: "colorInput", color_picker_color_value: "color" }, inline: true %>
There are a few style mismatches here. In order to clear out the default `styled_form` input styles, you can pass `inline: true` which tells our form builder to omit default styles: ```erb <%= f.text_field :color , data: { color_picker_target: "colorInput", color_avatar_target: "colorInput", color_picker_color_value: "color" }, inline: true %> ```
app/views/categories/_form.html.erb
@@ -17,1 +14,3 @@
</div>
<div class=" absolute z-50 bg-white p-4 border border-alpha-black-25 rounded-2xl shadow-xs h-fit left-66 top-24">
<div class="flex gap-2 flex-col mb-4" data-category-target="selection" style="<%= "display:none;" if @category.subcategory? %>">
<div data-category-target="pickerSection"></div>
zachgoll (Migrated from github.com) commented 2025-02-19 00:56:53 +08:00
Copy Link

We will need to update the structure of the details/summary so that we're not opening the details unless the user clicks the edit icon. Something along these lines:

 <div class="flex items-center justify-center">
        <div class="relative w-14 h-14 rounded-full flex justify-center items-center" style="background-color: <%= hex_with_alpha(category.color, 0.1) %>">
          <% if category.lucide_icon %>
            <%= icon(category.lucide_icon, size: "md", color: category.color) %>
          <% else %>
            <span class="uppercase" style="color: <%= category.color %>"><%= category.name&.first || "?" %></span>
          <% end %>

          <details>
            <summary class="cursor-pointer absolute -right-2 -bottom-2 flex justify-center items-center bg-gray-50 border-2 w-7 h-7 border-white rounded-full text-gray-500">
              <%= icon("pen", size: "sm") %>
            </summary>

            <div class="absolute left-20 top-10 z-50 bg-white p-4 border border-alpha-black-25 rounded-2xl shadow-xs h-fit" data-controller="color-picker">

https://github.com/user-attachments/assets/3c3a015f-e349-4e21-acd7-d6d5a1e10074

We will need to update the structure of the `details/summary` so that we're not opening the details unless the user clicks the edit icon. Something along these lines: ```erb <div class="flex items-center justify-center"> <div class="relative w-14 h-14 rounded-full flex justify-center items-center" style="background-color: <%= hex_with_alpha(category.color, 0.1) %>"> <% if category.lucide_icon %> <%= icon(category.lucide_icon, size: "md", color: category.color) %> <% else %> <span class="uppercase" style="color: <%= category.color %>"><%= category.name&.first || "?" %></span> <% end %> <details> <summary class="cursor-pointer absolute -right-2 -bottom-2 flex justify-center items-center bg-gray-50 border-2 w-7 h-7 border-white rounded-full text-gray-500"> <%= icon("pen", size: "sm") %> </summary> <div class="absolute left-20 top-10 z-50 bg-white p-4 border border-alpha-black-25 rounded-2xl shadow-xs h-fit" data-controller="color-picker"> ``` https://github.com/user-attachments/assets/3c3a015f-e349-4e21-acd7-d6d5a1e10074
app/views/categories/_form.html.erb
@@ -34,4 +63,3 @@
<div class="space-y-2">
<%= f.select :classification, [["Income", "income"], ["Expense", "expense"]], { label: "Classification" }, required: true %>
<%= f.text_field :name, placeholder: t(".placeholder"), required: true, autofocus: true, label: "Name", data: { color_avatar_target: "name" } %>
zachgoll (Migrated from github.com) commented 2025-02-19 01:05:39 +08:00
Copy Link

I think we should also throw a "clear icon" (i.e. nil radio button) somewhere here so the user can clear the icon if needed.

@justinfar I didn't see this in the designs. What is the preferred method for clearing an icon?

I think we should also throw a "clear icon" (i.e. `nil` radio button) somewhere here so the user can clear the icon if needed. @justinfar I didn't see this in the designs. What is the preferred method for clearing an icon?
zachgoll (Migrated from github.com) commented 2025-02-19 01:09:14 +08:00
Copy Link

Also, the selected icon needs to match the chosen color:
CleanShot 2025-02-18 at 12 08 40

Also, the selected icon needs to match the chosen color: ![CleanShot 2025-02-18 at 12 08 40](https://github.com/user-attachments/assets/4fc4fbba-f7a5-4e6d-8252-c26b281a2014)
app/views/shared/_color_avatar.html.erb
zachgoll (Migrated from github.com) commented 2025-02-19 00:45:24 +08:00
Copy Link

Throughout the codebase, we're using the color_avatar for categories, tags, and merchants.

Since we're adding specialization to the category avatar, I think we need to revert changes on this "shared" avatar and make a specific partial just for categories.

Throughout the codebase, we're using the `color_avatar` for categories, tags, and merchants. Since we're adding specialization to the category avatar, I think we need to revert changes on this "shared" avatar and make a specific partial just for categories.
zachgoll (Migrated from github.com) reviewed 2025-02-19 01:59:03 +08:00
app/javascript/controllers/color_avatar_controller.js
zachgoll (Migrated from github.com) commented 2025-02-19 01:59:03 +08:00
Copy Link

I think we should avoid observers like this and update this with one of the following approaches:

  1. Combine everything into a single controller (probably easiest)
  2. Use Stimulus outlets to update:
static outlets = ["color-avatar"];

// ...

initPicker() {
 // ...
 this.picker.on("change", (color, source, instance) => {
   const hexColor = color.toHEXA().toString();
   this.colorAvatarOutlet.updateAvatarColors(hexColor);
 }
}
I think we should avoid observers like this and update this with one of the following approaches: 1. Combine everything into a single controller (probably easiest) 2. Use Stimulus outlets to update: ```js static outlets = ["color-avatar"]; // ... initPicker() { // ... this.picker.on("change", (color, source, instance) => { const hexColor = color.toHEXA().toString(); this.colorAvatarOutlet.updateAvatarColors(hexColor); } } ```
zachgoll (Migrated from github.com) reviewed 2025-02-19 02:01:57 +08:00
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-19 02:01:57 +08:00
Copy Link

Validation states look good!

@justinfar can you do a quick check here on the design for us? Any changes you'd like?

CleanShot 2025-02-18 at 13 01 18

Validation states look good! @justinfar can you do a quick check here on the design for us? Any changes you'd like? ![CleanShot 2025-02-18 at 13 01 18](https://github.com/user-attachments/assets/e2c09419-b586-4063-8216-361d6c551cfb)
syedbarimanjan (Migrated from github.com) reviewed 2025-02-19 02:06:16 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-19 02:06:15 +08:00
Copy Link

We are using 5% for the background in here:
79e1a2c0ff/app/views/shared/_color_avatar.html.erb (L5)

We are using 5% for the background in here: https://github.com/maybe-finance/maybe/blob/79e1a2c0ffd5191fe6c040da68a311e1c1e6beb4/app/views/shared/_color_avatar.html.erb#L5
justinfar (Migrated from github.com) reviewed 2025-02-19 02:11:20 +08:00
app/javascript/controllers/color_picker_controller.js
justinfar (Migrated from github.com) commented 2025-02-19 02:11:20 +08:00
Copy Link

Re validation for contrast, if possible I'd prefer if the validation offered a way to automatically adjust (which basically darkens the color to an acceptable contrast ratio)

Text that should be used for validation is:
Poor contrast, choose darker color or auto-adjust.

CleanShot 2025-02-18 at 19 08 25

Re validation for contrast, if possible I'd prefer if the validation offered a way to automatically adjust (which basically darkens the color to an acceptable contrast ratio) Text that should be used for validation is: `Poor contrast, choose darker color or auto-adjust.` ![CleanShot 2025-02-18 at 19 08 25](https://github.com/user-attachments/assets/419d420b-ea27-454f-aa5b-d81329bb9cf1)
syedbarimanjan (Migrated from github.com) reviewed 2025-02-19 02:25:43 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-19 02:25:43 +08:00
Copy Link

Re validation for contrast, if possible I'd prefer if the validation offered a way to automatically adjust (which basically darkens the color to an acceptable contrast ratio)

I did think about this but got stuck on which color should I adjust to like should I just make the color black?

>Re validation for contrast, if possible I'd prefer if the validation offered a way to automatically adjust (which basically darkens the color to an acceptable contrast ratio) I did think about this but got stuck on which color should I adjust to like should I just make the color black?
justinfar (Migrated from github.com) reviewed 2025-02-19 02:32:52 +08:00
app/views/categories/_form.html.erb
@@ -34,4 +63,3 @@
<div class="space-y-2">
<%= f.select :classification, [["Income", "income"], ["Expense", "expense"]], { label: "Classification" }, required: true %>
<%= f.text_field :name, placeholder: t(".placeholder"), required: true, autofocus: true, label: "Name", data: { color_avatar_target: "name" } %>
justinfar (Migrated from github.com) commented 2025-02-19 02:32:52 +08:00
Copy Link

@zachgoll @syedbarimanjan re clearing an icon, for now let's just go with requiring an icon for all categories

The generic fallback icon should be the "shapes" icon (i.e the one below)
image

@zachgoll @syedbarimanjan re clearing an icon, for now let's just go with requiring an icon for all categories The generic fallback icon should be the "shapes" icon (i.e the one below) ![image](https://github.com/user-attachments/assets/ab86c647-2f9e-43d0-a023-b0aac5992e41)
zachgoll (Migrated from github.com) reviewed 2025-02-19 02:37:04 +08:00
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-19 02:37:04 +08:00
Copy Link

@syedbarimanjan I think we can just add a simple function that brings the hue down until it meets the contrast requirement. Something along these lines; we should be able to reuse quite a bit of the existing contrast checking code:

darkenColor([r, g, b, a]) {
  let darkened = [r, g, b, a];
  let backgroundColor = this.backgroundColor(darkened, 5);
  let contrastRatio = this.contrast(darkened, backgroundColor);
  
  // Keep darkening until we meet contrast requirements or hit black
  while (contrastRatio < 4.5 && (darkened[0] > 0 || darkened[1] > 0 || darkened[2] > 0)) {
    darkened = [
      Math.max(0, darkened[0] - 5),
      Math.max(0, darkened[1] - 5),
      Math.max(0, darkened[2] - 5),
      darkened[3]
    ];
    contrastRatio = this.contrast(darkened, backgroundColor);
  }
  
  return darkened;
}
@syedbarimanjan I think we can just add a simple function that brings the hue down until it meets the contrast requirement. Something along these lines; we should be able to reuse quite a bit of the existing contrast checking code: ```js darkenColor([r, g, b, a]) { let darkened = [r, g, b, a]; let backgroundColor = this.backgroundColor(darkened, 5); let contrastRatio = this.contrast(darkened, backgroundColor); // Keep darkening until we meet contrast requirements or hit black while (contrastRatio < 4.5 && (darkened[0] > 0 || darkened[1] > 0 || darkened[2] > 0)) { darkened = [ Math.max(0, darkened[0] - 5), Math.max(0, darkened[1] - 5), Math.max(0, darkened[2] - 5), darkened[3] ]; contrastRatio = this.contrast(darkened, backgroundColor); } return darkened; } ```
syedbarimanjan commented 2025-02-20 01:10:19 +08:00 (Migrated from github.com)
Copy Link

@zachgoll

https://github.com/user-attachments/assets/b55e5f6c-aed4-4529-8e4d-ef6786604995

@zachgoll https://github.com/user-attachments/assets/b55e5f6c-aed4-4529-8e4d-ef6786604995
zachgoll (Migrated from github.com) reviewed 2025-02-20 05:02:54 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Overall looking a lot better! Core flow works great.

Before we merge, I think there are a few general "cleanup" items to address (several mentioned in comments):

Model validation / migration

Currently, the Category class allows nil for a category. Per @justinfar comments about the shapes icon being our default, we'll need to:

  • Add a default value for lucide_icon of shapes on the Category table
  • Add a NOT NULL constraint on lucide_icon

Edge Cases in UI

  • When you edit a subcategory, users can change the color to be different than the parent. This is because we have before_create :inherit_color_from_parent in the model, but really, we need this to be before_commit to cover both create + update flows + hide the color option in the UI if it is editing a subcategory.
  • When a user selects a preset color, then choose a custom color, the preset color remains selected in the UI (see video below):

https://github.com/user-attachments/assets/2da509c8-17e2-4a98-b327-dbce680d61f8

Overall looking a lot better! Core flow works great. Before we merge, I think there are a few general "cleanup" items to address (several mentioned in comments): **Model validation / migration** Currently, the `Category` class allows `nil` for a category. Per @justinfar comments about the `shapes` icon being our default, we'll need to: - Add a default value for `lucide_icon` of `shapes` on the `Category` table - Add a NOT NULL constraint on `lucide_icon` **Edge Cases in UI** - When you edit a subcategory, users can change the color to be different than the parent. This is because we have `before_create :inherit_color_from_parent` in the model, but really, we need this to be `before_commit` to cover both create + update flows + hide the color option in the UI if it is editing a subcategory. - When a user selects a preset color, then choose a custom color, the preset color remains selected in the UI (see video below): https://github.com/user-attachments/assets/2da509c8-17e2-4a98-b327-dbce680d61f8
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-20 03:50:36 +08:00
Copy Link

Now that we've got this into one controller, I think we can rename it to something more specific to categories, such as category_controller.js or category_avatar_controller.js

Now that we've got this into one controller, I think we can rename it to something more specific to categories, such as `category_controller.js` or `category_avatar_controller.js`
zachgoll (Migrated from github.com) commented 2025-02-20 04:28:24 +08:00
Copy Link

We're mixing a good bit of view logic and contrast validation here. I think we can move a lot of this into the template which will simplify and make the controller a bit easier to read.

In the view, add the template:

<div data-color-picker-target="validationMessage" class="hidden self-start flex gap-1 items-center text-xs text-destructive">
  <span>Poor contrast, choose darker color or</span>
  <button class="underline" data-action="color-picker#autoAdjust">auto-adjust.</button>
</div>

And in the controller, we would just need something like this:

autoAdjust() {
  const currentRGBA = this.picker.getColor().toRGBA();
  let adjustedRGBA = currentRGBA;
  
  // Auto-darken logic

  this.picker.setColor(adjustedRGBA);
}

Then our validation function just becomes:

handleContrastValidation(contrastRatio, rgbacolor) {
  if (contrastRatio < 4.5) {
    this.colorInputTarget.setCustomValidity("Poor contrast, choose darker color or auto-adjust.");
    
    this.validationMessageTarget.classList.remove("hidden");
  } else {
    this.colorInputTarget.setCustomValidity("");
    this.validationMessageTarget.classList.add("hidden");
  }
}
We're mixing a good bit of view logic and contrast validation here. I think we can move a lot of this into the template which will simplify and make the controller a bit easier to read. In the view, add the template: ```erb <div data-color-picker-target="validationMessage" class="hidden self-start flex gap-1 items-center text-xs text-destructive"> <span>Poor contrast, choose darker color or</span> <button class="underline" data-action="color-picker#autoAdjust">auto-adjust.</button> </div> ``` And in the controller, we would just need something like this: ```js autoAdjust() { const currentRGBA = this.picker.getColor().toRGBA(); let adjustedRGBA = currentRGBA; // Auto-darken logic this.picker.setColor(adjustedRGBA); } ``` Then our validation function just becomes: ```js handleContrastValidation(contrastRatio, rgbacolor) { if (contrastRatio < 4.5) { this.colorInputTarget.setCustomValidity("Poor contrast, choose darker color or auto-adjust."); this.validationMessageTarget.classList.remove("hidden"); } else { this.colorInputTarget.setCustomValidity(""); this.validationMessageTarget.classList.add("hidden"); } } ```
zachgoll (Migrated from github.com) commented 2025-02-20 05:01:53 +08:00
Copy Link

We have several hardcoded values of 5%, 10%, and 15% throughout the controller. We should add a private method:

#backgroundColor(color) {
  return `color-mix(in oklab, ${color} 10%, transparent)`;
}

And then throughout the controller:

  • Remove all borders (those are outdated from prev designs)
  • Refer to this private method for avatar + icon backgrounds
We have several hardcoded values of `5%`, `10%`, and `15%` throughout the controller. We should add a private method: ```js #backgroundColor(color) { return `color-mix(in oklab, ${color} 10%, transparent)`; } ``` And then throughout the controller: - Remove all borders (those are outdated from prev designs) - Refer to this private method for avatar + icon backgrounds
app/views/categories/_color_avatar.html.erb
@@ -0,0 +5,4 @@
class="w-14 h-14 flex items-center justify-center rounded-full"
style="background-color: color-mix(in oklab, <%= category.color %> 10%, transparent); color: <%= category.color %>">
<%= lucide_icon(category.lucide_icon, class: "w-8 h-8") %>
</span>
zachgoll (Migrated from github.com) commented 2025-02-20 04:57:17 +08:00
Copy Link

Once we get the NULL constraint set on the category model, we should be able to update this partial to this:

<%# locals: (category:) %>

<span 
      data-color-picker-target="avatar"
      class="w-14 h-14 flex items-center justify-center rounded-full"
      style="color: <%= category.color %>; background-color: color-mix(in oklab, <%= category.color %> 10%, transparent)">
  <%= lucide_icon(category.lucide_icon, class: "w-8 h-8") %>
</span>
Once we get the NULL constraint set on the category model, we should be able to update this partial to this: ```erb <%# locals: (category:) %> <span data-color-picker-target="avatar" class="w-14 h-14 flex items-center justify-center rounded-full" style="color: <%= category.color %>; background-color: color-mix(in oklab, <%= category.color %> 10%, transparent)"> <%= lucide_icon(category.lucide_icon, class: "w-8 h-8") %> </span> ```
syedbarimanjan (Migrated from github.com) reviewed 2025-02-20 22:56:44 +08:00
app/javascript/controllers/color_picker_controller.js
syedbarimanjan (Migrated from github.com) commented 2025-02-20 22:56:44 +08:00
Copy Link

How about new_category_controller.js

How about ```new_category_controller.js```
syedbarimanjan commented 2025-02-21 06:27:14 +08:00 (Migrated from github.com)
Copy Link

@zachgoll

@zachgoll
zachgoll (Migrated from github.com) reviewed 2025-02-21 23:29:34 +08:00
app/javascript/controllers/color_avatar_controller.js
@@ -23,3 +23,3 @@
const color = e.currentTarget.value;
this.avatarTarget.style.backgroundColor = `color-mix(in srgb, ${color} 5%, white)`;
this.avatarTarget.style.backgroundColor = `color-mix(in srgb, ${color} 10%, white)`;
this.avatarTarget.style.borderColor = `color-mix(in srgb, ${color} 10%, white)`;
zachgoll (Migrated from github.com) commented 2025-02-21 22:34:25 +08:00
Copy Link

This needs to be added back to support the color changes for merchants and tags.

This needs to be added back to support the color changes for merchants and tags.
app/javascript/controllers/color_picker_controller.js
zachgoll (Migrated from github.com) commented 2025-02-21 10:09:56 +08:00
Copy Link

This controller should handle both creates/updates so I think category_controller.js is more appropriate

This controller should handle both creates/updates so I think `category_controller.js` is more appropriate
app/javascript/controllers/new_category_controller.js
zachgoll (Migrated from github.com) commented 2025-02-21 23:18:41 +08:00
Copy Link

This method can be simplified to just this since setting the color will trigger all these reassignments:

autoAdjust(e) {
  const currentColor = this.picker.getColor();
  const adjustedRGBA = this.darkenColor(currentColor).toString();
  this.picker.setColor(adjustedRGBA);
}

Then simply work with the HSVaColor object directly in darkenColor method:

darkenColor(color) {
  let darkened = color.toRGBA();
  const backgroundColor = this.backgroundColor(darkened, 10);
  let contrastRatio = this.contrast(darkened, backgroundColor);

  while (
    contrastRatio < 4.6 &&
    (darkened[0] > 0 || darkened[1] > 0 || darkened[2] > 0)
  ) {
    darkened = [
      Math.max(0, darkened[0] - 10),
      Math.max(0, darkened[1] - 10),
      Math.max(0, darkened[2] - 10),
      darkened[3],
    ];
    contrastRatio = this.contrast(darkened, backgroundColor);
  }

  return `rgba(${darkened.join(", ")})`;
}

Not a huge deal, but I think the more we can pass around the HSVaColor object throughout the controller, the more clear and concise the code becomes and the less conversions we have to deal with.

This method can be simplified to just this since setting the color will trigger all these reassignments: ```js autoAdjust(e) { const currentColor = this.picker.getColor(); const adjustedRGBA = this.darkenColor(currentColor).toString(); this.picker.setColor(adjustedRGBA); } ``` Then simply work with the `HSVaColor` object directly in `darkenColor` method: ```js darkenColor(color) { let darkened = color.toRGBA(); const backgroundColor = this.backgroundColor(darkened, 10); let contrastRatio = this.contrast(darkened, backgroundColor); while ( contrastRatio < 4.6 && (darkened[0] > 0 || darkened[1] > 0 || darkened[2] > 0) ) { darkened = [ Math.max(0, darkened[0] - 10), Math.max(0, darkened[1] - 10), Math.max(0, darkened[2] - 10), darkened[3], ]; contrastRatio = this.contrast(darkened, backgroundColor); } return `rgba(${darkened.join(", ")})`; } ``` Not a huge deal, but I think the more we can pass around the `HSVaColor` object throughout the controller, the more clear and concise the code becomes and the less conversions we have to deal with.
zachgoll (Migrated from github.com) commented 2025-02-21 23:29:28 +08:00
Copy Link

We should pass these into the controller as values so that if we ever change these presets in the Category class, we don't have to come here and update this again to preserve functionality:

static values = {
  presetColors: Array,
};

<div data-controller="color-picker" data-color-picker-preset-colors-value="<%= Category::COLORS %>">
We should pass these into the controller as values so that if we ever change these presets in the Category class, we don't have to come here and update this again to preserve functionality: ```js static values = { presetColors: Array, }; ``` ```erb <div data-controller="color-picker" data-color-picker-preset-colors-value="<%= Category::COLORS %>"> ```
app/views/categories/_color_avatar.html.erb
@@ -0,0 +5,4 @@
class="w-14 h-14 flex items-center justify-center rounded-full"
style="background-color: color-mix(in oklab, <%= category.color %> 10%, transparent); color: <%= category.color %>">
<%= lucide_icon(category.lucide_icon, class: "w-8 h-8") %>
</span>
zachgoll (Migrated from github.com) commented 2025-02-21 23:23:52 +08:00
Copy Link

Make sure the icon size and alpha channel are matching what I've suggested here so we're in parity with the Figma designs (in oklab, transparent, w-8)

Make sure the icon size and alpha channel are matching what I've suggested here so we're in parity with the Figma designs (in oklab, transparent, w-8)
db/migrate/20250220200735_add_default_lucide_icon_to_categories.rb
@@ -0,0 +20,4 @@
WHERE lucide_icon = 'shapes'
SQL
end
end
zachgoll (Migrated from github.com) commented 2025-02-21 22:47:11 +08:00
Copy Link

This migration will fail on our production data which has categories with nil values in this column. We need to first set existing categories to the shapes icon, then we can safely add these NULL changes here.

This migration will fail on our production data which has categories with `nil` values in this column. We need to first set existing categories to the `shapes` icon, then we can safely add these NULL changes here.
db/schema.rb
@@ -10,7 +10,7 @@
#
zachgoll (Migrated from github.com) commented 2025-02-21 22:50:13 +08:00
Copy Link

Looks like this is from a stale migration of some sort. You can run bin/rails db:migrate:reset to get rid of this.

Looks like this is from a stale migration of some sort. You can run `bin/rails db:migrate:reset` to get rid of this.
zachgoll (Migrated from github.com) reviewed 2025-02-22 04:43:10 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

After all comments are addressed and merge conflicts resolved, we'll run the tests and get this merged.

Functionality looks good!

After all comments are addressed and merge conflicts resolved, we'll run the tests and get this merged. Functionality looks good!
app/javascript/controllers/category_controller.js
@@ -0,0 +1,206 @@
import { Controller } from "@hotwired/stimulus"
zachgoll (Migrated from github.com) commented 2025-02-22 04:38:57 +08:00
Copy Link

We can remove this now

We can remove this now
app/views/categories/_color_avatar.html.erb
@@ -0,0 +5,4 @@
class="w-14 h-14 flex items-center justify-center rounded-full"
style="background-color: color-mix(in oklab, <%= category.color %> 10%, transparent); color: <%= category.color %>">
<%= lucide_icon(category.lucide_icon, class: "w-8 h-8") %>
</span>
zachgoll (Migrated from github.com) commented 2025-02-22 04:31:34 +08:00
Copy Link

@syedbarimanjan can you update this to match the code I posted?

@syedbarimanjan can you update this to match the code I posted?
zachgoll (Migrated from github.com) approved these changes 2025-02-24 23:23:41 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Code looks good! Looks like there is a failing check, so once resolved we're good to merge.

Code looks good! Looks like there is a failing check, so once resolved we're good to merge.
syedbarimanjan commented 2025-02-24 23:53:38 +08:00 (Migrated from github.com)
Copy Link

@zachgoll resolved the failing check and merged with main to resolve conflicts.

@zachgoll resolved the failing check and merged with main to resolve conflicts.
zachgoll commented 2025-02-25 00:07:20 +08:00 (Migrated from github.com)
Copy Link

@syedbarimanjan great, nice work! Let me know if you have any issues with the bounty payment.

@syedbarimanjan great, nice work! Let me know if you have any issues with the bounty payment.
zachgoll (Migrated from github.com) approved these changes 2025-02-25 00:07:59 +08:00
Sign in to join this conversation.
Reviewers
No Reviewers
justinfar
syedbarimanjan
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label 🙋 Bounty claim
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#1857
Delete Branch "Improvement/new-add-category-flow"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?