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(Slack Node): Fix @version issue in param editor #12089

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anatolinicolae
Copy link

@anatolinicolae anatolinicolae commented Dec 7, 2024

Slack node version was bumped to 2.3 but this one keeps the prop field hidden in the UI

Summary

Can't pick file prop in the UI when using "Upload file" node for Slack, so I'm adding the new version on prop show condition.

Parameters Settings
image image

Related PRs

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@CLAassistant
Copy link

CLAassistant commented Dec 7, 2024

CLA assistant check
All committers have signed the CLA.

@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request in linear Issue or PR has been created in Linear for internal review labels Dec 7, 2024
@Joffcom
Copy link
Member

Joffcom commented Dec 7, 2024

Hey @anatolinicolae,

Thanks for the PR, We have created "GHC-578" as the internal reference to get this reviewed.

One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team.

@Joffcom
Copy link
Member

Joffcom commented Dec 10, 2024

A better solution might be to use the "gte" condition so it applies to all future versions as well.

@anatolinicolae
Copy link
Author

@Joffcom that's what I wanted to do at first, but yeah don't really know how impacting will future slack lib updates be. Do we want to yolo it with the "gte" condition?

@Joffcom
Copy link
Member

Joffcom commented Dec 12, 2024

@anatolinicolae yes that is exactly what we want to do as this change was introduced as part of Slacks api update so this won't change again unless Slacks changes something.

@GimmyHchs
Copy link

GimmyHchs commented Dec 12, 2024

@anatolinicolae Hi, I think you can fix the same issue in FileDescription.ts.
Please refer to the duplicate closed PR. Thanks!

@anatolinicolae
Copy link
Author

Should be reflecting pretty much all changes from #12177 now, thanks @GimmyHchs! 🙌

@GimmyHchs
Copy link

GimmyHchs commented Dec 13, 2024

Should be reflecting pretty much all changes from #12177 now, thanks @GimmyHchs! 🙌

@anatolinicolae Hi, I found an issue during manual testing and hope it can be addressed together. I apologize for any inconvenience caused. 🙏

Here is a reference for the fix commit: GimmyHchs@35906bf

before after
CleanShot 2024-12-13 at 11 54 13 CleanShot 2024-12-13 at 11 50 26

btw, I think the PR title can be changed to fix(Slack Node) ... to better align with the convention.

@anatolinicolae anatolinicolae changed the title fix: Allow prop to show on Upload file for Slack new version fix(Slack Node): Fix @version issue in param editor Dec 13, 2024
@anatolinicolae
Copy link
Author

Cherry-picked your commit 👍

@anatolinicolae anatolinicolae force-pushed the fix-slack-prop branch 2 times, most recently from 01803ac to f94e8c1 Compare December 13, 2024 13:32
@GimmyHchs
Copy link

Hi @Joffcom
Any updates on this pull request?

@anatolinicolae anatolinicolae force-pushed the fix-slack-prop branch 3 times, most recently from f2a48ca to d013b1d Compare January 15, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member in linear Issue or PR has been created in Linear for internal review node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants