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

fix: fixed branch selector #415

Merged
merged 4 commits into from
Jun 22, 2024
Merged

Conversation

ayush3160
Copy link
Contributor

Branch names now are rendering as removable-chips after reloading page.

Linked Issue(s)

fixes #331

Proposed changes (including videos or screenshots)

2024-06-04.18-45-50.mp4

@jayantbh
Copy link
Contributor

jayantbh commented Jun 5, 2024

Thanks for both of your PRs, @ayush3160
Someone from the team will check it out asap.

@e-for-eshaan
Copy link
Contributor

Hey @ayush3160 . Thanks for the contribution!

Pulled your PR for testing, this behaviour seems incorrect. Custom branch names should render only if custom branches are selected, other wise the prod branches and all branches selectors shall remain highlighted.

image

Here, no custom branches were selected, yet the input is filled. This can be fixed by checking if prod-branches or all-branches are selected. if yes, then the state for the input must be empty.

feel free to ask more questions, we'll be glad to help!

@ayush3160
Copy link
Contributor Author

Hi @e-for-eshaan , Thanks for reviewing I missed the case of prod and all branches mode but I have fixed it in above commit please have a look and let me know if any other changes are needed.

e-for-eshaan
e-for-eshaan previously approved these changes Jun 21, 2024
)
}
);
depFn(localBranchNames.set, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

localBranchNames.set should be included in the dependency array of the setProdStateBranchNames callback

@e-for-eshaan
Copy link
Contributor

Hey @ayush3160 ! Sorry for the late response, but your PR looks promising, just tested it locally.
Mentioned a minor change, can be merged right after this!

@e-for-eshaan e-for-eshaan dismissed their stale review June 21, 2024 15:01

Mentioned changes

@ayush3160
Copy link
Contributor Author

Thanks for reviewing @e-for-eshaan , I have done the changes please check once.

Copy link
Contributor

@e-for-eshaan e-for-eshaan left a comment

Choose a reason for hiding this comment

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

LGTM! approving this

@e-for-eshaan e-for-eshaan merged commit 2037f6a into middlewarehq:main Jun 22, 2024
1 check passed
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.

Selected branch-name in selector and refreshing, removes the chips of the selected branch-names
3 participants