-
Notifications
You must be signed in to change notification settings - Fork 429
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: Implement be search and lazy loading for GitHub resources #3987
feat: Implement be search and lazy loading for GitHub resources #3987
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
Flagsmith Feature
|
Flagsmith Feature Segment
|
The Segment Override
|
The feature flag
|
How did you test this code?The Flagsmith GitHub App is configured to point Vercel's staging deployment for testing as the post-installation landing page and the corresponding (staging) API server as the GitHub events webhook endpoint. NOTE: The current integration works properly with environments using the versioning v1. The automated comments are not yet properly working with v2 so please use a project with only v1-versioning environments Test checklist (have fun)
Thanks for participating! Here you have a bonus track test with a quiz and a beer on reward if you answer the quiz right in a comment (only if you didn't find any issues so far ;) and have approved the PR ;) ) Bonus Track Test Quiz |
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've been through both the code, and the test plan and everything seems good to me. Really nice work here @novakzaballa, the UX for the integration is super smooth. I've added a few findings below that I think we should perhaps look at separately but nothing that should block us merging this PR. Perhaps @novakzaballa you can create the relevant GH issues to tackle them separately?
Minor (can be implemented in a separate PR)
- We should maybe have a manual 'Refresh Repositories' button next to the dropdown when linking a repository. When you add another repository to the Github side, you have to hard refresh the page before it is picked up in Flagsmith.
- The content / styling of the comments could be more consistent. Particularly between the linked and unlinked comments. See screenshot 1 below.
General comments questions
- It's odd to me that the Flagsmith organisation shows 'Configure' when I open the integration dialogue, but I can still add it to my project as well. This is probably because some other project has an integration with it. I'm not sure there's anything we'll be able to do, but maybe we could add some content to the documentation about it. A note on this is that when you click 'delete integration' from one of the projects, I think it removes it from all projects. Maybe we should at least have a warning on the 'Delete integration' that this is the case?
- It's a bit jarring that the 'links' tab is in the edit feature modal, which is only view in the context of an environment. This is an inherent problem in Flagsmith (e.g. the 'settings' tab is also project-wise) so not a major issue here but certainly feels jarring.
Screenshots
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
How did you test this code?
The Flagsmith GitHub App is configured to point Vercel's staging deployment for testing as the post-installation landing page and the corresponding (staging) API server as the GitHub events webhook endpoint.
NOTE: The current integration works properly with environments using the versioning v1. The automated comments are not yet properly working with V2
Please use Vercel's staging deployment for testing (Visit Preview) and use a project with only versioning_v1 environments.
Detailed testing checklist/instructions on the comment at the bottom of this PR: #3987 (comment)