-
Notifications
You must be signed in to change notification settings - Fork 447
feat: integrations onboarding #5497
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # frontend/common/utils/utils.tsx
# Conflicts: # frontend/web/styles/_variables.scss
# Conflicts: # frontend/common/types/requests.ts # frontend/common/types/responses.ts
# Conflicts: # frontend/common/types/responses.ts
# Conflicts: # frontend/common/types/requests.ts # frontend/common/types/responses.ts # frontend/common/utils/utils.tsx # frontend/web/components/pages/HomeAside.tsx # frontend/web/components/pages/HomePage.js # frontend/web/project/api.js
Flagsmith feature linked:
|
Flagsmith Feature
|
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.
A couple of comments, not critical but if maybe you have time to split up a bit the component. Then it should be quickly good to go
I just looked at IntegrationSelect, it needs rebased to remove the welcome page diff
onClick={() => { | ||
setShowCustomTool(false) | ||
setSelected(selected.concat([customTool])) | ||
}} |
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.
I'd just reset the state setCustomTool here so if you open it again it's empty
</div> | ||
</div> | ||
<hr className='mb-5 mt-0' /> | ||
<div className='sticky d-flex justify-content-end gap-4'> |
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.
const skip = () => | ||
updateTools({ | ||
tools: { | ||
completed: true, | ||
integrations: [], | ||
}, | ||
}).then(onComplete) | ||
const submit = () => | ||
updateTools({ | ||
tools: { | ||
completed: true, | ||
integrations: selected, | ||
}, | ||
}).then(onComplete) |
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.
Are you fine with using async await syntax here ?
onComplete: () => void | ||
} | ||
|
||
const integrationSummaries: IntegrationSummary[] = [ |
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.
Can we put them in a separate data/constant file to light up the file ?
title: 'SumoLogic', | ||
}, | ||
] | ||
const categoryDescriptions: Record< |
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.
same for this one
if (!integrationsData) { | ||
return [] as typeof integrationSummaries | ||
} | ||
const parsed = Object.values(JSON.parse(integrationsData)).concat( |
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.
Can we find a more self-explanatory name like parsedCategories
// Initialize search as an empty string | ||
const [search, setSearch] = useState<string>('') | ||
const integrationsData = Utils.getFlagsmithValue('integration_data') | ||
const [category, setCategory] = useState<string>(ALL_CATEGORY) |
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 could rename it to selectedCategory, setSelectedCategory do not mix in case of using a map
return sortBy(filtered, 'title') as typeof integrationSummaries | ||
}, [integrationsData, category, search]) | ||
|
||
const [selected, setSelected] = useState<string[]>([]) |
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.
Same if you don't mind to rename to something like selectedIntegrations
if (isSelected) { | ||
setSelected( | ||
selected.filter( | ||
(selectedItem) => selectedItem !== v.title, | ||
), | ||
) | ||
} else { | ||
setSelected(selected.concat(v.title)) | ||
} |
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 might put it in its own handler above to have the logic isolated
# Conflicts: # frontend/common/types/requests.ts # frontend/common/types/responses.ts # frontend/e2e/tests/initialise-tests.ts # frontend/web/components/App.js # frontend/web/components/onboarding/GettingStartedResource.tsx # frontend/web/components/onboarding/data/onboarding.data.tsx # frontend/web/components/pages/CreateOrganisationPage.js # frontend/web/components/pages/GettingStartedPage.tsx # frontend/web/components/pages/HomeAside.tsx # frontend/web/components/pages/IntegrationsPage.tsx # frontend/web/styles/project/_project-nav.scss # frontend/web/styles/project/_utils.scss
…rations-onboarding
Flagsmith Feature
|
Flagsmith Feature
|
Flagsmith Feature Segment
|
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
How did you test this code?
Please describe.