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

DOP-4549: Branches and repos in separate dropdowns in slack deploy dialog #1029

Merged
merged 56 commits into from
May 7, 2024

Conversation

anabellabuckvar
Copy link
Contributor

@anabellabuckvar anabellabuckvar commented Apr 15, 2024

Stories/Links:

DOP-4549

Notes

This PR:

  1. updates slack.ts v2 with to be consistent with v1 code due to changes implemented in DOP-4481 that created a Deploy All button( although v2 is essentially unnecessary and never executes, this was done so there are no errors and make future work more seamless)
  2. Refactors some of the slack deploy code and reconfigures the slack deploy modal such that Docs Site Versions are grouped by Docs Site. This will make it easier to find and deploy sites, and also solves the issue of the 100 item limit in slack repos as long as there are fewer than 100 versions for any given site.
Screenshot 2024-05-01 at 11 34 16 AM

Example Build

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@anabellabuckvar anabellabuckvar changed the title Dop 4549 DOP-4549: Branches and repos in separate dropdowns in slack deploy dialog Apr 15, 2024
Copy link

Your feature branch infrastructure has been deployed!

Your webhook URL is: https://es3go1ktj7.execute-api.us-east-2.amazonaws.com/prod/webhook/githubEndpoint/trigger/build

For more information on how to use this endpoint, follow these instructions.

@anabellabuckvar anabellabuckvar marked this pull request as ready for review May 1, 2024 15:59
Copy link
Contributor

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

I'm confused at how this solves the >100 choices in the dropdown list. At first glance, it seems to actually bring the problem back? Because the inactive branches now are sorted only by their repoName rather than all the way to the bottom. I feel like I'm missing something, though!

Comment on lines 51 to 58
//sort the options by version number
const repoOption = {
label: {
type: 'plain_text',
text: repoName,
},
options: options.sort((branchOne, branchTwo) => {
return branchTwo.text.text
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you move the comment down to the options line?

@anabellabuckvar
Copy link
Contributor Author

I'm confused at how this solves the >100 choices in the dropdown list. At first glance, it seems to actually bring the problem back? Because the inactive branches now are sorted only by their repoName rather than all the way to the bottom. I feel like I'm missing something, though!

Fair question!! Each group can have 100 choices, so now we are only limited to 100 versions per repo as opposed to 100 versions total in the dialog.

@mmeigs mmeigs self-requested a review May 6, 2024 16:56
Copy link
Contributor

@branberry branberry left a comment

Choose a reason for hiding this comment

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

Hey Anabella! This looks really good! Just a few small comments

options: options.sort((branchOne, branchTwo) => {
return branchTwo.text.text
.toString()
.replace(/\d+/g, (n) => +n + 100000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can forgo the .replace statement as I think the comparison should work as is. Here's a quick example I used:
image
It seems that with or without the replace, we get the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right for those examples! Unfortunately, when we are sorting versions like v7.18 and v7.2, if we don't use the .replace statement localeCompare will sort the versions as if v7.2 is greater than v7.18, which we don't want

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha! That makes sense

}
return entitledBranches.sort();
return repoOptions.sort((repoOne, repoTwo) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: This can be simplified to

return repoOptions.sort((repoOne, repoTwo) => repoOne.label.text.localeCompare(repoTwo.label.text);

@branberry branberry self-requested a review May 7, 2024 09:14
Copy link
Contributor

@branberry branberry left a comment

Choose a reason for hiding this comment

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

Hey Anabella! Aside from the small nit, this looks good to me.

@anabellabuckvar anabellabuckvar merged commit 3a93a7b into main May 7, 2024
9 checks 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.

3 participants