-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
Removed vultr server and associated DNS entries |
There was a problem hiding this 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
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Outdated
Show resolved
Hide resolved
editor.planx.uk/src/pages/FlowEditor/components/Settings/ServiceSettings.tsx
Show resolved
Hide resolved
@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? |
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. |
|
||
<PublicLink | ||
isFlowPublished={isFlowPublished} | ||
status={statusForm.values.status} |
There was a problem hiding this comment.
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?
status={statusForm.values.status} | |
status={flowStatus} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
70c7208
to
8d98492
Compare
Converted to Draft as I work on test coverage |
1f4323d
to
cdd2315
Compare
There was a problem hiding this 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 💪
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
orpublished
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.
I also created to new variables in the main
<ServiceSettings>
component -publishedLink
andsubdomainLink
to control the creation of each link.The
subdomainLink
is built differently to thepublishedLink
as thepublishedLink
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 builtsubdomainLink
felt more explicit for me with exposedflowSlug
in the url. I've highlighted them in the snippet belowFor 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.