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.

Sign up for GitHub

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

Jump to bottom

fix: save workflow button not visible on dev env #14910

Merged
merged 5 commits into from May 9, 2024
Merged

fix: save workflow button not visible on dev env #14910

merged 5 commits into from May 9, 2024

Conversation

mhetreayush
Copy link
Contributor

@mhetreayush mhetreayush commented May 7, 2024 •
edited

What does this PR do?

This PR renders the "Save" button in edit/create-workflow flow on the development environment.

Fixes #14909

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  1. Click on "Workflows" in the sidebar
  2. Click "+ Create a workflow" or edit a workflow.
  3. On the top right corner, the "Save" button should be visible.
image

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel Vercel
Copy link

vercel bot commented May 7, 2024

@mhetreayush is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot requested a review from a team May 7, 2024 06:13
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label May 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@dosubot dosubot bot added workflows area: workflows, automations 🐛 bug Something isn't working labels May 7, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented May 7, 2024

Graphite Automations

"Add community label" took an action on this PR • (05/07/24)

1 label was added to this PR based on Keith Williams's automation.

"Add consumer team as reviewer" took an action on this PR • (05/07/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@CLAassistant
Copy link

CLAassistant commented May 7, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@mhetreayush mhetreayush force-pushed the save-workflow-button-not-visible-on-dev-env branch 2 times, most recently from 0cad9fd to 6c939c1 Compare May 7, 2024 14:23
CarinaWolli
CarinaWolli requested changes May 7, 2024
View reviewed changes
packages/features/ee/workflows/pages/workflow.tsx Outdated
@@ -277,7 +279,7 @@ function WorkflowPage() {
}
hideHeadingOnMobile
heading={
session.data?.hasValidLicense &&
(IS_DEV || session.data?.hasValidLicense) &&
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the IS_DEV check. We an always show the save button, all we need is the banner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added because:

  1. On the local environment, session.data?.hasValidLicense evaluates to false, hence making the heading prop undefined.
  2. The shell component only renders the CTA if the heading is defined.
  3. Hence, in order to pass in the heading, this check was added.

reference: https://github.com/mhetreayush/cal.com/blob/22ccb69c6e5ba5d6ea24b24bd2535b2677abe8d9/packages/features/shell/Shell.tsx#L1030-L1066

And wrapping the complete component inside the changes the UI, making it look like this:
image

If this is okay, we can remove the complete check: IS_DEV || session.data?.hasValidLicense and render the save button regardless of the validLicense

Or

We can just remove the hasValidLicense check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CarinaWolli Let me know what you think about this.

Copy link
Member

Choose a reason for hiding this comment

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

we can remove the complete IS_DEV || session.data?.hasValidLicense check. I'd love to make it look the same as for routing forms:
Screenshot 2024-05-08 at 10 57 16 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

@keithwillcode keithwillcode added this to the v4.1 milestone May 8, 2024
@mhetreayush
Copy link
Contributor Author

mhetreayush commented May 8, 2024 •
edited

@CarinaWolli Fixed with 557ae4c

image

Also fixed it on the /workflows page:

  • Initial:
image
  • With fix:
image

@mhetreayush mhetreayush force-pushed the save-workflow-button-not-visible-on-dev-env branch from 653a6c2 to 5dacd9e Compare May 8, 2024 16:10
@mhetreayush
Copy link
Contributor Author

Let me know if the changes look good @CarinaWolli

CarinaWolli
CarinaWolli approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@CarinaWolli CarinaWolli left a comment

Choose a reason for hiding this comment

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

Changes look good now, thank you 🙏

@CarinaWolli CarinaWolli enabled auto-merge (squash) May 9, 2024 17:28
@CarinaWolli CarinaWolli merged commit 097a69d into calcom:main May 9, 2024
32 of 39 checks passed
@mhetreayush mhetreayush deleted the save-workflow-button-not-visible-on-dev-env branch May 10, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarinaWolli CarinaWolli CarinaWolli approved these changes

Assignees
No one assigned
Labels
🐛 bug Something isn't working community Created by Linear-GitHub Sync workflows area: workflows, automations
Projects
None yet
Milestone
v4.1
Development

Successfully merging this pull request may close these issues.

The "Save Workflow" button is not visible on development environment
4 participants
@mhetreayush @CLAassistant @CarinaWolli @keithwillcode