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

Update to gin 3.0-rc13 #852

Draft
wants to merge 2 commits into
base: 3.x
Choose a base branch
from
Draft

Update to gin 3.0-rc13 #852

wants to merge 2 commits into from

Conversation

paul121
Copy link
Member

@paul121 paul121 commented Jun 28, 2024

Opening this issue to track any changes we need to support gin 3.0-rc12.

Since we are currently on 3.0-rc10 this also includes various improvements and bugfixes from 3.0-rc11

But most notably a new feature that moves all form action buttons to a top sticky header: https://www.drupal.org/project/gin/issues/3356717

  • So far I've found one bug related to this change and submitted a fix: https://www.drupal.org/project/gin/issues/3440148
  • The feature is turned off by default but can be turned on in user overrides, so we should make sure it doesn't break anything too much. It seems that it might default to "on" in an upcoming release so it might be good to embrace the change.
  • I have mixed feelings but am coming around to it. It's a nice improvement and would create some consistency across all forms but certainly is a significant change for existing users.

@mstenta
Copy link
Member

mstenta commented Jul 8, 2024

Looks like rc13 was released today.

@mstenta
Copy link
Member

mstenta commented Jul 8, 2024

I have mixed feelings but am coming around to it. It's a nice improvement and would create some consistency across all forms but certainly is a significant change for existing users.

Can you post a screenshot of what this looks like?

It seems that it might default to "on" in an upcoming release so it might be good to embrace the change.

Agreed.

@paul121
Copy link
Member Author

paul121 commented Jul 10, 2024

Here are some screenshots:

Asset edit form (save + delete)

asset-more-actions

Inventory quick form (submit)

inventory-submit

Editing a configurable quick form (save + delete)

The "enabled" checkbox should not be here (doesn't work) and is due to the error I mentioned above.

edit-quick-form

Module settings (install modules)

install-modules-submit

@paul121
Copy link
Member Author

paul121 commented Jul 11, 2024

But most notably a new feature that moves all form action buttons to a top sticky header:

Chatting today, lets add an update hook to enable this by default, but open in a separate PR and not merge until Gin upgrades this feature from "experimental" and makes it a default.

@mstenta
Copy link
Member

mstenta commented Jul 15, 2024

Chatting today, lets add an update hook to enable this by default, but open in a separate PR and not merge until Gin upgrades this feature from "experimental" and makes it a default.

Update: I think we amended this plan on the dev call (correct me if I'm wrong @paul121).

We will update to the new version but we will NOT make an update hook to enable sticky buttons... yet. Instead we will make a follow-up draft PR that enables them, but wait until that becomes the official default in Gin before we merge it here.

The thinking is: right now it's "experimental" in Gin, which means there is still a chance it WON'T be added or made the default. So if we add an update hook now, and then it goes away, we may need another update hook to remove it. By waiting we a) let it get more testing in the wild before adopting, and b) potentially save ourselves needing to undo it with another update hook if upstream decides not to merge it. Maybe overly safe but seems like the right way to proceed here.

So I think this can be merged (right @paul121?) - but perhaps we should just knock off that draft PR now so that we don't forget.

@paul121
Copy link
Member Author

paul121 commented Jul 15, 2024

Update: I think we amended this plan on the dev call (correct me if I'm wrong @paul121).

Sorry yes - we're saying the same thing! I just left that quick note during the dev call and wasn't very clear

So I think this can be merged (right @paul121?) - but perhaps we should just knock off that draft PR now so that we don't forget.

I think we should wait for a fix or patch for the bug I mentioned above. But I don't think we're in a rush to upgrade tho, I don't remember seeing any other improvements or fixes that are very important

@mstenta mstenta added this to the v3.3.0 milestone Jul 31, 2024
@paul121
Copy link
Member Author

paul121 commented Aug 29, 2024

I'll investigate rc13 and see if my patch for above mentioned issue might still apply...

@paul121
Copy link
Member Author

paul121 commented Sep 18, 2024

I'll investigate rc13 and see if my patch for above mentioned issue might still apply...

There was a conflict and I had to rebase this patch onto the latest gin changes. I've also pinged the Gin group in slack to see if we can get a review on this. I would love to have someone review this before we include this patch in FarmOS since it is a moderately involved/potentially very significant change.

It also looks like there has been a decent number of bug fixes since rc13 so I think we're just better off waiting for a new release at this point.

@mstenta
Copy link
Member

mstenta commented Oct 31, 2024

Our current version of Gin is causing a problem with the admin permissions page. See: #887

Gin 3.0-rc13 fixes it. I'll link this PR to #887 so that it closes when we merge this.

@mstenta mstenta linked an issue Oct 31, 2024 that may be closed by this pull request
@mstenta mstenta changed the title Update to gin 3.0-rc12 Update to gin 3.0-rc13 Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Permissions page is blank
2 participants