Skip to content

Fix/app multiple installs#3045

Merged
zomars merged 12 commits intomainfrom
fix/app-multiple-installs
Jun 13, 2022
Merged

Fix/app multiple installs#3045
zomars merged 12 commits intomainfrom
fix/app-multiple-installs

Conversation

@alannnc
Copy link
Contributor

@alannnc alannnc commented Jun 13, 2022

What does this PR do?

Fixed some conditionals on app page, to display that they can only be installed once and for calendars they can be installed multiple times

Fixes # (issue)

Loom Video: https://www.loom.com/share/2a35144a84f942418f75d916b7e668cd

Environment: Staging(main branch) / Production

Type of change

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

How should this be tested?

  • Install Wipe My Cal app, return to app page and it should say Installed
  • Install any calendar app, return to app page and it should display 1 installed connection with the add another button.

Checklist

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

@vercel
Copy link

vercel bot commented Jun 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Jun 13, 2022 at 9:09PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
docs ⬜️ Ignored (Inspect) Jun 13, 2022 at 9:09PM (UTC)
swagger ⬜️ Ignored (Inspect) Jun 13, 2022 at 9:09PM (UTC)
ui ⬜️ Ignored (Inspect) Jun 13, 2022 at 9:09PM (UTC)

email,
tos,
privacy,
multipleInstall = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New flag to know if an app can be installed multiple times

{!isLoading ? (
isGlobal || installedAppCount > 0 ? (
<div className="space-x-3">
{isLoading && <SkeletonButton width="24" height="10" />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed ternary operation for explicit conditional

Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with the ternary?

@alannnc alannnc marked this pull request as ready for review June 13, 2022 07:16
@alannnc alannnc requested review from hariombalhara and leog June 13, 2022 07:16
Copy link
Contributor

@leog leog left a comment

Choose a reason for hiding this comment

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

Great job @alannnc. Take a look at some comments I left, and also, I don't think we should be committing the yarn.lock file 🤔

@leog leog mentioned this pull request Jun 13, 2022
1 task
Copy link
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

I'd rather not add any more things to metadata as is going to be replace by the App model in the DB. Why not user the calendar category for now as it was before? Or there are any non-calendar apps that can be installed multiple times?

Co-authored-by: Leo Giovanetti <hello@leog.me>
@alannnc
Copy link
Contributor Author

alannnc commented Jun 13, 2022

I'd rather not add any more things to metadata as is going to be replace by the App model in the DB. Why not user the calendar category for now as it was before? Or there are any non-calendar apps that can be installed multiple times?

Yeah, will switch to calendar instead then. Didn't know about upcoming AppModel in DB

@zomars zomars merged commit 8d893d3 into main Jun 13, 2022
@zomars zomars deleted the fix/app-multiple-installs branch June 13, 2022 21:06
@zomars zomars mentioned this pull request Jun 15, 2022
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants