Skip to content

Unified Resources UI: Various Fixes#32064

Merged
avatus merged 10 commits intomasterfrom
avatus/unified_fixes
Sep 19, 2023
Merged

Unified Resources UI: Various Fixes#32064
avatus merged 10 commits intomasterfrom
avatus/unified_fixes

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Sep 18, 2023

This PR is a collection of a few fixes

  • Sort alphabetically regardless of case
    Screenshot 2023-09-18 at 2 06 20 PM

  • Removes the old spinning indicator at the bottom of the unified resources list

  • Includes friendly name in the app icon guess

  • adds Select All/Clear All buttons to the type filter menu, as well as a selected filter count and indiciator
    Screenshot 2023-09-18 at 12 07 03 PM

Comment on lines +155 to +162
const handleSelectAll = () => {
setKinds(kindOptions.map(k => k.value));
};

const handleClearAll = () => {
setKinds([]);
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These methods seem a bit simple atm, but when we start to add more filter types besides just kinds they will grow a bit.

onClick={handleOpen}
>
Type
Types {kindsFromParams.length > 0 ? `(${kindsFromParams.length})` : ''}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Similarly, this will change once we add more filters but abstracting now seemed a bit frivolous

Comment thread api/types/resource.go
}

return stringCompare(nameA, nameB, isDesc)
return stringCompare(strings.ToLower(nameA), strings.ToLower(nameB), isDesc)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I believe I have the test testing what we want 91cb348

Copy link
Copy Markdown
Contributor

@kimlisa kimlisa Sep 18, 2023

Choose a reason for hiding this comment

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

i wanted to test this change out in storybook, but i think most of it got broken along the way. doesn't seem like this PR would've broke it, or am i missing something? (i saw these stories in working state like couple weeks ago)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Storybook is working for me. What errors are you seeing?

Copy link
Copy Markdown
Contributor

@kimlisa kimlisa Sep 18, 2023

Choose a reason for hiding this comment

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

weird, i tried it again (404)
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I believe that is related to changes made here #30964

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm... Folks, I don't want to sound cliche, but it works for me. :D

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holy cow, I think I know what the issue is. I just noticed that you are using your machine's explicit IP address, and I was always using 127.0.0.1 instead. If I substitute my machine's IP, it ends up on 404, too. It looks like that mock service worker Storybook plugin fails to load resources with a different IP. I'll take a look and see if it can be configured another way, since I'd really like to avoid moving away from it (it was the only way I could get both API calls and SVG loading to work). In the meantime, please use 127.0.0.1.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really know how to fix it. :( It might be related to security restrictions (apparently, service workers are not allowed for pages not running on HTTPS if they're not accessed using localhost URLs). The mystery is why then other tests work, so I'm more inclined to believe that this is plugin-specific (other tests use MSW directly). I have a more important thing to do at the moment (backporting that PR mentioned above), but perhaps I'll give it one more try in the afternoon. Perhaps upgrading Storybook will change something; the version we're using is a bit old and ratchety.

@avatus avatus enabled auto-merge September 18, 2023 22:14
@avatus avatus added this pull request to the merge queue Sep 19, 2023
Merged via the queue into master with commit b894559 Sep 19, 2023
@avatus avatus deleted the avatus/unified_fixes branch September 19, 2023 14:44
@public-teleport-github-review-bot
Copy link
Copy Markdown

@avatus See the table below for backport results.

Branch Result
branch/v14 Failed

@public-teleport-github-review-bot
Copy link
Copy Markdown

@avatus See the table below for backport results.

Branch Result
branch/v14 Failed

avatus added a commit that referenced this pull request Sep 19, 2023
* Remove old indicator from unified resources

* Use lowercase for unified name sort

* Include friendly name in app icon guess

* Add filters exist indicator

* Remove uppercase from filter buttons and add filter count

* Remove indicator import

* Resource custom sort by contained resource

* Add unified name compare test
github-merge-queue Bot pushed a commit that referenced this pull request Sep 19, 2023
* Remove old indicator from unified resources

* Use lowercase for unified name sort

* Include friendly name in app icon guess

* Add filters exist indicator

* Remove uppercase from filter buttons and add filter count

* Remove indicator import

* Resource custom sort by contained resource

* Add unified name compare test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants