Skip to content
This repository has been archived by the owner on Jun 14, 2019. It is now read-only.

Tech Preview feature not working for Integration Editor #409

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

Tech Preview feature not working for Integration Editor #409

wants to merge 30 commits into from

Conversation

kahboom
Copy link
Contributor

@kahboom kahboom commented Jun 4, 2019 •
edited
Loading

Created another version of ConnectionsWithToolbar per @riccardo-forina 's suggestion.

Screenshot 2019-06-04 16 06 10

fix #374

@kahboom kahboom requested a review from riccardo-forina June 4, 2019 15:15
@kahboom kahboom requested a review from gashcrumb June 4, 2019 15:15
@riccardo-forina
Copy link
Collaborator

PR Storybook available here

gashcrumb
gashcrumb suggested changes Jun 4, 2019
View reviewed changes
Copy link
Collaborator

@gashcrumb gashcrumb left a comment

Choose a reason for hiding this comment

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

The name, it has to change 😄

riccardo-forina
riccardo-forina suggested changes Jun 4, 2019
View reviewed changes
Copy link
Collaborator

@riccardo-forina riccardo-forina left a comment

Choose a reason for hiding this comment

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

I suggest EditorStep for the name.

app/ui-react/syndesis/src/modules/connections/components/ConnectionsWithToolbarNew.tsx Outdated Show resolved Hide resolved
app/ui-react/syndesis/src/modules/connections/components/ConnectionsWithToolbarNew.tsx Outdated Show resolved Hide resolved
@kahboom
Copy link
Contributor Author

kahboom commented Jun 4, 2019

Alright, here are the changes I've made, based on feedback, that seem to work (albeit some are hacky):

  • Rename to EditorStepsWithToolbar
  • Move to integrations module for reuse
  • Duplicate of Connections component called EditorSteps that uses the IUIIntegrationStep interface and EditorStepsWithToolbar
  • Adjusted the typings to comply (please note the use of as Blah[] a few times, not ideal but it works for now)
  • Rename connections to steps to reflect data more accurately

And still using the .map() part for the filteredAndSortedConnections, as we needed it.

riccardo-forina
riccardo-forina reviewed Jun 4, 2019
View reviewed changes
Copy link
Collaborator

@riccardo-forina riccardo-forina left a comment

Choose a reason for hiding this comment

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

Just a quick check, I’ll test this more thoroughly tomorrow

app/ui-react/syndesis/src/modules/integrations/components/editor/EditorSteps.tsx Outdated Show resolved Hide resolved
app/ui-react/syndesis/src/modules/integrations/components/editor/EditorStepsWithToolbar.tsx Outdated Show resolved Hide resolved
app/ui-react/syndesis/src/modules/integrations/components/editor/EditorStepsWithToolbar.tsx Outdated Show resolved Hide resolved
@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@kahboom kahboom force-pushed the 374-config-req-bug branch from e302395 to 1885990 Compare June 5, 2019 09:08
@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

riccardo-forina
riccardo-forina suggested changes Jun 6, 2019
View reviewed changes
Copy link
Collaborator

@riccardo-forina riccardo-forina left a comment

Choose a reason for hiding this comment

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

It doesn't work

doesntwork

@kahboom kahboom changed the title Tech Preview feature not working for Integration Editor WIP - Tech Preview feature not working for Integration Editor Jun 6, 2019
@riccardo-forina
Copy link
Collaborator

Actually, this needs a rebase, the error I posted above is because we have an extension on staging and this PR is based on a version of the app that was bugging out with that.

@kahboom kahboom force-pushed the 374-config-req-bug branch from fed2c74 to 22674e3 Compare June 6, 2019 16:25
@kahboom
Copy link
Contributor Author

kahboom commented Jun 6, 2019

Screenshot 2019-06-06 17 20 10

No additional or changes to interfaces, no .map, no referencing the Connection interface from what I can see, just added an additional check and used @gashcrumb 's suggestion of the getMetadataValue helper. I did read @riccardo-forina 's suggestion several times. I do understand that this is a not a Connection and instead comprises connections, extensions, and stepKinds, where IUIStep normalizes it. This is using that interface. Something happens in mergeConnectionsSources where a connection property gets added to each object. I understand we can't rely on that, but it's still there, and needs to be iterated over to reach metadata, so I've added another check in EditorSteps.tsx to check if that property exists. Short of that I'm out of ideas, so let me know what exactly would need to change, if anything.

@kahboom kahboom changed the title WIP - Tech Preview feature not working for Integration Editor Tech Preview feature not working for Integration Editor Jun 6, 2019
@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@riccardo-forina

This comment has been minimized.

Sign in to view
@kahboom

This comment has been minimized.

Sign in to view
@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@kahboom kahboom force-pushed the 374-config-req-bug branch from 06dfeb4 to bf39ab3 Compare June 7, 2019 09:37
kahboom added 27 commits June 14, 2019 12:43
@kahboom
create EditorSteps and EditorStepsWithToolbar for reuse in integratio…
4592d77 
…ns module with IUIIntegrationStep type
@kahboom
Rework the typings for compatibility
901cadb 
Lint fixes
@kahboom
remove deleteConnections, menu, notifications
9ed4aaa
@kahboom
rename connections to editorSteps
d79d02d 
Re-add map for flag to work properly

Lint fix

Update the icon
@kahboom
Use getMetadataValue to check for metadata, rm map
bee0d5d
@kahboom
Check for connection prop
ac3f257
@kahboom
Update name for props, interface
336c44e 
Rm getConnectionHref
@kahboom
Refactor check for configurationRequired
fd07268
@kahboom
Rm unnecessary default values obj
8555271 
Lint fixes
@kahboom
Rm unused Connection model
6cfa27a
@kahboom
Create reusable check for tech preview flag
6dc03e3
@kahboom
Refactor Connections.tsx to use helper functions
27d96cb
@kahboom
Rename check for techPreview
3581bc2 
Re-add checks until further clarification
@kahboom
Rm if statements, add configRequired prop to useConnection
922cbd6
@kahboom
Pass connector directly to isTechPreview
9477252
@kahboom
Rm reference to isConfigurationRequired from EditorSteps
214ce60
@kahboom
Rm reference to helpers from EditorSteps
abcdffc
@kahboom
Add transformConnectorsResponse and other improvements
a745687
@kahboom
Add configRequired and isTechPreview props to utils and set to false …
f3aa020 
…for Extensions and Steps
@kahboom
Make props for IConnectionOverview optional
8fd0c36
@kahboom
Move IConnectionOverview to extra.d.ts and rm board prop
c104c7f
@kahboom
Add techPreview to useConnector
20c98c6
@kahboom
Refactor tech preview check in ConnectorsPage.tsx
97df3bd
@kahboom
Refactor the bejesus out of Connection and ConnectionOverview
f315a09
@kahboom
stricter interfaces, upgrade to IConnectionOverview in Connections.ts…
de5945f 
…x, more refactoring in useConnection
@kahboom
adapt utils toUIStep to accommodate for isTechPreview and configRequi…
539aea9 
…red flags
@kahboom
Rename configRequired to isConfigRequired
c8cfc7e
@kahboom kahboom force-pushed the 374-config-req-bug branch from 14e2d6e to c8cfc7e Compare June 14, 2019 11:44
@kahboom kahboom mentioned this pull request Jun 14, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gashcrumb gashcrumb gashcrumb requested changes

@riccardo-forina riccardo-forina riccardo-forina requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pr/wip status/review-requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kahboom @riccardo-forina @gashcrumb @paoloantinori