-
Notifications
You must be signed in to change notification settings - Fork 321
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
[UI] Refactor Banks Page #1652
[UI] Refactor Banks Page #1652
Conversation
Buggy, ugly code but I'm going to the pub here's a start EDIT: it's ready for a review! |
804ef4f
to
5f96376
Compare
5f96376
to
106d42d
Compare
<p class="text-secondary">Banks are like folders. You can do the following on this page:</p> | ||
<ul class="text-secondary"> | ||
<li>Create new banks</li> | ||
<li>Add content: Add content to each bank</li> | ||
<li>Search by content: Find banks that contain a certain piece of content</li> | ||
</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this up lol. Someone should come up with better copy to describe these concepts.
I think all the pages should have a h1 title and a description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and was tempted to give you and @levi-cloudflare a nudge in that direction. I'm fine to land-and-iterate on the content.
Why I agree:
- The primary purpose of the UI should be to help a non-technical person understand what HMA is doing for showcase demos. We assume anyone using it "for real" will only use the API
- Therefore, the UI should primarily focus on introducing concepts (e.g. "Bank" terminology.
const highlightMatchedResults = (result) => { | ||
// Render matched banks | ||
result.banks.forEach((bankName) => { | ||
document.getElementById(`bank_item_${bankName}`).classList.add("bg-light"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish this was a darker highlight, but I don't want to mess around with adding custom colors to this and the other provided boostrap colors are ugly in this context lol. I'll leave it up to ya'll
<tr id="bank_item_{{bank['name']}}"> | ||
<th scope="row" style="background-color: transparent;">{{ loop.index }}</th> | ||
<td style="background-color: transparent;">{{ bank['name'] }}</td> | ||
<td style="background-color: transparent;">{{ bank['matching_enabled_ratio'] }}</td> | ||
<td style="background-color: transparent;"> | ||
{% with bank_title=bank['name'] %} | ||
{% include 'components/banks/add_to_bank_form.html.j2' %} | ||
{% endwith %} | ||
</td> | ||
</tr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually, these will link to their own page with stats and such.
Leaving that up to ya'll too
@@ -0,0 +1,37 @@ | |||
{# Copyright (c) Meta Platforms, Inc. and affiliates. #} | |||
<button type="button" class="btn btn-light" data-bs-toggle="modal" data-bs-target="#{{bank_title}}">Add content</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish this was an icon and not "Add content" but I didn't want to mess around with icon libraries either lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - we want to keep things as simple as possible, which sometimes means that things may look a little odd because we are trading off against adding more dependencies or custom styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually break something with these UI restyles, but it's labor intensive to click on everything, and it looks like you did all the tests I would have run. Let's land-and-iterate.
<p class="text-secondary">Banks are like folders. You can do the following on this page:</p> | ||
<ul class="text-secondary"> | ||
<li>Create new banks</li> | ||
<li>Add content: Add content to each bank</li> | ||
<li>Search by content: Find banks that contain a certain piece of content</li> | ||
</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and was tempted to give you and @levi-cloudflare a nudge in that direction. I'm fine to land-and-iterate on the content.
Why I agree:
- The primary purpose of the UI should be to help a non-technical person understand what HMA is doing for showcase demos. We assume anyone using it "for real" will only use the API
- Therefore, the UI should primarily focus on introducing concepts (e.g. "Bank" terminology.
@@ -0,0 +1,37 @@ | |||
{# Copyright (c) Meta Platforms, Inc. and affiliates. #} | |||
<button type="button" class="btn btn-light" data-bs-toggle="modal" data-bs-target="#{{bank_title}}">Add content</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - we want to keep things as simple as possible, which sometimes means that things may look a little odd because we are trading off against adding more dependencies or custom styling.
Summary
This PR refactors the bank grid page, match form, create bank form, and add content form
Test Plan
Screenshots: