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

feat: Shareable Public Link in Service Settings #3525

Merged
merged 24 commits into from
Aug 20, 2024
Merged

Conversation

RODO94
Copy link
Contributor

@RODO94 RODO94 commented Aug 16, 2024

What does this PR do?

Follows work completed here: #3483

And addresses the Trello ticket: https://trello.com/c/k8cQojk7/2958-share-service-link-in-editor

I have added a new section to the Status card of the Service Settings page. The new section displays a Public Link with the intent to expose a Team's subdomain or published link (in both cases the query param ?analytics=false as been excluded from the link).

To achieve this, I added three new components <PublicLink> ,<TitledLink> and <CopyButton> to simplify the code and contain certain parts of the logic within contextual components.

Main piece of logic in these components is the handling of what link to show when, and when to show it active or inactive.

const PublicLink: React.FC<{
  isFlowPublished: boolean;
  status: FlowStatus;
  subdomain: string;
  publishedLink: string;
}> = ({ isFlowPublished, status, subdomain, publishedLink }) => {

  const isFlowPublic = isFlowPublished && status === "online"; 
  // this variable helps contain boolean logic for active / inactive
  
  const hasSubdomain = Boolean(subdomain); 
  // this variable makes subdomain strictly true / false instead of truthy / falsey

  switch (true) {
    case isFlowPublic && hasSubdomain:
      return <TitledLink isActive={true} link={subdomain} />;
    case isFlowPublic && !hasSubdomain:
      return <TitledLink isActive={true} link={publishedLink} />;
    case !isFlowPublic && hasSubdomain:
      return <TitledLink isActive={false} link={subdomain} />;
    case !isFlowPublic && !hasSubdomain:
      return <TitledLink isActive={false} link={publishedLink} />;
  }
  // decided to contain the subdomain and publishedLink within this component instead of...
  // adding it to the main component and just passing a "link" prop down to this component...
  // since it contains the logic in one place instead of two
};

I also created to new variables in the main <ServiceSettings> component - publishedLink and subdomainLink to control the creation of each link.

The subdomainLink is built differently to the publishedLink as the publishedLink was taken from the main index which controls the FlowEditor, I'd be interested to know the differences in how they are built, and if I should be harmonising these. The way I built subdomainLink felt more explicit for me with exposed flowSlug in the url. I've highlighted them in the snippet below

   const publishedLink = `${window.location.origin}${rootFlowPath( false)}/published`; 
  // above is taken from flowEditor/index.tsx line 43

  const subdomainLink = teamDomain && `https://${teamDomain}/${flowSlug}`;

For displaying help text to the user, I have created a function managing a switch statement, adjusting help based on if the flow is Online and/or Published. Simplest approach seemed to let the function control the string that feeds in to the standard <Typography> being used in the section of the card rather than the function return a react component for each case.

@RODO94 RODO94 requested a review from a team August 16, 2024 11:17
Copy link

github-actions bot commented Aug 16, 2024

Removed vultr server and associated DNS entries

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Really appreciate this refactor - it's so much easier to read, the styling and code patterns feel much more consistent, and it achieves the original user story really cleanly !

I did experience a couple unexpected navigation things when actually clicking the links though! Plus a few other comments throughout

@RODO94 RODO94 requested a review from jessicamcinchak August 16, 2024 13:56
@jessicamcinchak
Copy link
Member

jessicamcinchak commented Aug 16, 2024

@RODO94 when I see a "container unhealthy" or "in use elsewhere" error on Vultr - I typically destroy then re-run failed action only ! Can stem from many commits close together I think and overlapping "update" jobs colliding. Want to try those steps and see it if clears up CI?

Screenshot from 2024-08-16 16-06-27

@jessicamcinchak
Copy link
Member

I'd also personally still like to see a bit of test coverage here ahead of a final review please !! With your latest refactor, it should be much more straightforward to write tests that confirm links are formatted as expected, correct text is displaying etc.

@RODO94
Copy link
Contributor Author

RODO94 commented Aug 16, 2024


<PublicLink
isFlowPublished={isFlowPublished}
status={statusForm.values.status}
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be the required fix here?

Suggested change
status={statusForm.values.status}
status={flowStatus}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DafyddLlyr I may have missed something, what is it fixing? I think this is the change we chatted about on Slack?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - this is it - I linked to this comment in the Slack chat, just wanted a note here also linked to the specific line.

image

Apologies for any confusion, please mark as resolved if sorted! 👍

@RODO94 RODO94 force-pushed the rory/shareable-links-V02 branch from 70c7208 to 8d98492 Compare August 19, 2024 14:55
@RODO94 RODO94 marked this pull request as draft August 19, 2024 15:09
@RODO94
Copy link
Contributor Author

RODO94 commented Aug 19, 2024

Converted to Draft as I work on test coverage

@RODO94 RODO94 force-pushed the rory/shareable-links-V02 branch from 1f4323d to cdd2315 Compare August 20, 2024 12:30
@RODO94 RODO94 marked this pull request as ready for review August 20, 2024 14:31
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

We talked through this one this afternoon - all final comments / suggestions / questions resolved there - looks good to merge to me !!

Thanks again for wading through all the initial design uncertainty with this task and seeing through this new solution - comprehensive test coverage and all 💪

@RODO94 RODO94 merged commit 04b0997 into main Aug 20, 2024
12 checks passed
@RODO94 RODO94 deleted the rory/shareable-links-V02 branch August 20, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants