feat(client): Add file type icons #8
Conversation
There was a problem hiding this comment.
Looking good!
Just a few ideas to see if we can refactor the file types a bit more, but I man not sure if this will work and/or produce more organized code. No need to spend a lot of time on it.
I have taken the liberty to update your PR on main for the CI failures.
src/labicons.ts
Outdated
| export const getIcon = (iconName: string, isLight: boolean) => { | ||
| let icon: LabIcon | undefined; | ||
| switch (iconName) { | ||
| case "parquet": | ||
| icon = getLabIcon(iconName, isLight ? parquetSvgLight : parquetSvgDark); | ||
| break; | ||
| case "arrowipc": | ||
| icon = getLabIcon(iconName, isLight ? arrowIPCSvg : arrowIPCDarkSvg); | ||
| break; | ||
| case "orc": | ||
| icon = getLabIcon(iconName, isLight ? orcLightSvg : orcDarkSvg); | ||
| break; | ||
| case "avro": | ||
| icon = getLabIcon(iconName, isLight ? avroSvg : avroSvg); | ||
| break; | ||
| } | ||
| return icon; |
There was a problem hiding this comment.
I believe this could be handled with global constants since we are aware in the code of which one to use. For instance, we already know we need a parquet icon when calling this function for the Parquet file type.
const PARQUET_ICON = getLabIcon("parquet", parquetSvgLight);
const PARQUET_DARK_ICON = getLabIcon("parquet", parquetSvgDark);
...Or maybe as many functions as file types
export const getParquetIcon = (isLight: boolean) => {
...
}
...This is so we avoid confusion between all the strings we have for the file types.
There was a problem hiding this comment.
Hello @AntoinePrv I tried to use the approach of constants but it did not work. I created videos where I showed the issue with it. Instead of this, to fix it, I created functions for each file type.
Screencast from 2025-12-15 16-33-17.webm
Screencast from 2025-12-15 16-29-47.webm
Thank you for your attention :)
AntoinePrv
force-pushed
the
add-icons
branch
from
e3ccf70 to
401297f
Compare
AntoinePrv
changed the title
|
Integration tests are taking a while and not testing much yet. Going forward with merge. |
2 participants
This PR has the solution of #7
The result:
Screencast from 2025-12-15 17-20-24.webm