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

Unable to load grid when using external postgres #13714

Merged

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented May 17, 2024

Description

This PR tackles the following points:

  1. Support time with time zone/time without time zone PG columns
  2. Fix time-only formatting (storing time format instead of timestamp ISO formats)
  3. Using native input type="time" in the frontend, to support native features and simplifying the code
  4. Fixing timezone issues that was incrementing time wrongly

Addresses

Screenshots

Empty datetime picker

image

Filled datetime picker

image

Empty time picker

image

Filled time picker

image

Launchcontrol

Fix time-only column issues

@adrinr adrinr marked this pull request as ready for review May 20, 2024 15:05
@adrinr adrinr requested a review from a team as a code owner May 20, 2024 15:05
@adrinr adrinr requested review from mike12345567 and aptkingston and removed request for a team May 20, 2024 15:05
@adrinr adrinr added the feature-branch Release this PR code into a feature branch label May 20, 2024
Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

Nice changes! Makes sense to use the native time input - that was an oversight on my part.

I get this ugly black time icon in Chromium though, and only 1 dash after the colon:

image

I wonder if we can improve the look at all?

packages/bbui/src/helpers.js Show resolved Hide resolved
packages/bbui/src/Form/Core/DatePicker/TimePicker.svelte Outdated Show resolved Hide resolved
Copy link
Collaborator

@samwho samwho left a comment

Choose a reason for hiding this comment

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

Approving backend half of this, with a few small comments.

@adrinr adrinr requested review from aptkingston and samwho May 21, 2024 14:55
Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

LGTM! I'm happy to leave the offset timezone changes in if that fixed an issue for you. There is probably some scenario that works with the old code, but hopefully usage of time-only is low and people won't run into issues.

Eventually we can hopefully move to a proper type for times and avoid that altogether anyway 👍

@adrinr adrinr enabled auto-merge May 22, 2024 08:23
@adrinr adrinr merged commit 5d29cfc into master May 22, 2024
10 checks passed
@adrinr adrinr deleted the budi-8195/unable-to-load-grid-when-using-external-postgres branch May 22, 2024 08:33
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-branch Release this PR code into a feature branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants