Skip to content
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

Fix Drive table #12397

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from
Open

Fix Drive table #12397

wants to merge 44 commits into from

Conversation

somebody1234
Copy link
Contributor

@somebody1234 somebody1234 commented Mar 3, 2025

Pull Request Description

  • Add background to "hidden columns" menu in the top right of the Drive table
  • Fix arrows being on separate line on "name" and "modified" columns in Drive table
  • Fix https://github.com/enso-org/cloud-v2/issues/1777
    • Adjust alignment of text beside icons

Important Notes

None

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@somebody1234 somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. --bug Type: bug g-dashboard labels Mar 3, 2025
@somebody1234 somebody1234 changed the title Wip/sb/flat assets fixes Fix Drive table Mar 3, 2025
Copy link

github-actions bot commented Mar 3, 2025

🧪 Storybook is successfully deployed!

📊 Dashboard:

@PabloBuchu
Copy link
Contributor

Something is still broken with the scroll bar

Screenshot 2025-03-03 at 10 43 48

@@ -337,9 +337,7 @@ const ButtonContent = memo(function ButtonContent(props: ButtonContentProps) {
styles={styles}
hideLoader={hideLoader}
/>
<Text color="inherit" truncate="1" className={styles.text()}>
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a span with className={styles.text()}, haven't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, i tried extracting the css directly and it doesn't seem to work.
try running this commit in dev (staging env) and create a project execution (no repeats to minimize load, but long timezone to (try to) trigger the truncate class):

438c4f3

}

/** A text display with an icon. */
export function IconDisplay<IconType extends string>(props: IconDisplayProps<IconType>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

when I told about patterns I meant only the ICON_DISPLAY_STYLES part,

Copy link
Contributor Author

@somebody1234 somebody1234 Mar 5, 2025

Choose a reason for hiding this comment

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

i guess, but i do want a dedicated component to avoid having to rewrite the markup (and get it right) every time, unless you have a better idea

Copy link
Contributor

@MrFlashAccount MrFlashAccount Mar 7, 2025

Choose a reason for hiding this comment

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

When we didn't have any abstractions for styling, we used to add purely stylistic components like <Stack /> <Grid /> or similar. Now we have tailwind-variants which allows us to encapsulate visuals and make them reusable, this makes the stylistic components obsolete (thats why we don't have them). I'm not against this component, but lets add a ./components/patterns/ directory and move the ICON_DISPLAY_STYLES there if you don't mind. (I'd like to rename ICON_DISPLAY_STYLES to ICON_WITH_TEXT_PATTERN)

See: https://github.com/chakra-ui/chakra-ui/blob/main/packages/react/src/components/flex/flex.tsx - this better be a construtor fn that returns styles : https://panda-css.com/docs/concepts/patterns#flex

data-current
aria-current="page"
textSelection="none"
elementType="a"
icon={icon}
isCurrent={isCurrent}
Copy link
Contributor

Choose a reason for hiding this comment

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

IconDisplay isCurrent ? isDisabled ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm. oops

@@ -104,14 +103,11 @@ function DashboardInner(props: DashboardProps) {
const initialProjectName = initialLocalProjectPath != null ? null : initialProjectNameRaw

const categoriesAPI = useCategoriesAPI()

Copy link
Contributor

Choose a reason for hiding this comment

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

Where did we move the refetch directories though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we no longer batch refetch directories since there is no longer a tree, no? so only one directory ever needs to be refetched at any time

Copy link
Contributor

@MrFlashAccount MrFlashAccount Mar 7, 2025

Choose a reason for hiding this comment

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

Hm, that makes sense, but we need to be sure we added the refetch in options object, but I have no idea how to avoid turning the listDirectory options constructor into a hook (as we still rely on feature-flags here). So if you have an idea how to tackle this, I'm happy to delete these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think my preferred solution would be to add a hook to return the required variables (enable refetch and refetch interval), and add an extra parameter to the constructor (required so that the user must explicitly opt out) that takes in the return type of that hook. thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case it's easy to forget to add this, the same query folks use in the Editor IIRC, and I think it makes sense to avoid spreading logic across the app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alternatively we can just have a hook useListDirectory maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: easy to forget, there's a mandatory new property to the listDirectory options constructor so at the very least it'd be noticeable if someone intentionally avoids passing it

@somebody1234
Copy link
Contributor Author

somebody1234 commented Mar 12, 2025

@MrFlashAccount a heads up that this PR fixes default weights in Text variant="...", but that does mean some changes like displays in the asset panel when a project is not selected now (correctly??) uses font-bold instead of font-medium (the default that overrode the base weight for each variant)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug CI: No changelog needed Do not require a changelog entry for this PR. g-dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants