Skip to content

Replace yank button with a drop down menu #11366

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

Merged
merged 8 commits into from
Jun 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions app/components/privileged-action.hbs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
{{#if this.isPrivileged}}
<div>
<div ...attributes>
{{yield}}
</div>
{{else if this.canBePrivileged}}
{{#if (has-block 'placeholder')}}
<div>
<div ...attributes>
{{yield to='placeholder'}}
</div>
{{else}}
<div local-class='placeholder'>
<div local-class='placeholder' ...attributes>
<fieldset data-test-placeholder-fieldset disabled="disabled">
{{yield}}
</fieldset>
Expand All @@ -18,7 +18,7 @@
</div>
{{/if}}
{{else}}
<div>
<div ...attributes>
{{#if (has-block 'unprivileged')}}
{{yield to='unprivileged'}}
{{/if}}
Expand Down
15 changes: 13 additions & 2 deletions app/components/version-list/row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,18 @@
{{/if}}
</div>

<PrivilegedAction @userAuthorised={{this.isOwner}}>
<YankButton @version={{@version}} local-class="yank-button" />
<PrivilegedAction @userAuthorised={{this.isOwner}} local-class="actions">
<Dropdown local-class="dropdown" data-test-actions-menu as |dd|>
<dd.Trigger @hideArrow={{true}} local-class="trigger" data-test-actions-toggle>
{{svg-jar "ellipsis-circle" local-class="icon"}}
<span class="sr-only">Actions</span>
</dd.Trigger>

<dd.Menu local-class="menu" as |menu|>
<menu.Item>
<YankButton @version={{@version}} class="button-reset" local-class="menu-button" />
</menu.Item>
</dd.Menu>
</Dropdown>
</PrivilegedAction>
</div>
51 changes: 46 additions & 5 deletions app/components/version-list/row.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -235,11 +235,52 @@
margin-top: var(--space-2xs);
}

.yank-button {
position: relative;
margin-left: var(--space-xs);
.actions {
display: flex;
}

@media only screen and (max-width: 550px) {
display: none;
.dropdown {
display: flex;
font-size: initial;
line-height: 1rem;
}

.icon {
width: 2em;
height: auto;
}

.trigger {
background: none;
border: none;
padding: 0;
border-radius: 99999px;
color: var(--grey600);

:hover {
border-radius: 99999px;
color: var(--grey900);
background-color: white;
}
}

.menu {
top: 100%;
right: 0;
min-width: max-content;
}

.menu-button {
align-items: center;
gap: var(--space-2xs);
cursor: pointer;
text-transform: capitalize;

/* This duplicates the styles in .button[disabled] as there's no
* obvious way to compose them, given the target selectors. */
&[disabled] {
background: linear-gradient(to bottom, var(--bg-color-top-light) 0%, var(--bg-color-bottom-light) 100%);
color: var(--disabled-text-color) !important;
cursor: not-allowed;
}
}
2 changes: 0 additions & 2 deletions app/components/yank-button.hbs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{{#if @version.yanked}}
<button
type="button"
class="button button--small"
...attributes
data-test-version-unyank-button={{@version.num}}
disabled={{@version.unyankTask.isRunning}}
Expand All @@ -16,7 +15,6 @@
{{else}}
<button
type="button"
class="button button--small"
...attributes
data-test-version-yank-button={{@version.num}}
disabled={{@version.yankTask.isRunning}}
Expand Down
15 changes: 10 additions & 5 deletions e2e/acceptance/sudo.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ test.describe('Acceptance | sudo', { tag: '@acceptance' }, () => {
await expect(page.locator('[data-test-disable-admin-actions]')).toHaveCount(0);
await expect(page.locator('[data-test-enable-admin-actions]')).toHaveCount(0);

// Assert that there's no dropdown menu toggle, disabled, enabled, or in any state.
await expect(page.locator('[data-test-actions-toggle]')).toHaveCount(0);
// Assert that there's no yank button, disabled, enabled, or in any state.
await expect(page.locator('[data-test-version-yank-button="0.1.0"]')).toHaveCount(0);
});
Expand All @@ -53,14 +55,13 @@ test.describe('Acceptance | sudo', { tag: '@acceptance' }, () => {
await expect(page.locator('[data-test-enable-admin-actions]')).toBeVisible();

// Test that the fieldset is present and disabled.
await expect(page.locator('[data-test-placeholder-fieldset]')).toBeVisible();
await expect(page.locator('[data-test-placeholder-fieldset]').first()).toBeVisible();
// NOTE: `toBeDisabled()` is not working as expected because the element is not a form control element.
// Ref: https://github.com/microsoft/playwright/issues/13583#issuecomment-1101704985
await expect(page.locator('[data-test-placeholder-fieldset]')).toHaveAttribute('disabled', 'disabled');
await expect(page.locator('[data-test-placeholder-fieldset]').first()).toHaveAttribute('disabled', 'disabled');

// From the perspective of the actual button, it isn't disabled, even though
// the fieldset effectively makes it unclickable.
await expect(page.locator('[data-test-version-yank-button="0.1.0"]')).toBeVisible();
await expect(page.locator('[data-test-actions-toggle]')).toBeDisabled();
await expect(page.locator('[data-test-version-yank-button="0.1.0"]')).toBeHidden();
});

test('admin user can enter sudo mode', async ({ page, msw }) => {
Expand Down Expand Up @@ -93,6 +94,8 @@ test.describe('Acceptance | sudo', { tag: '@acceptance' }, () => {
});
expect(seen).toBe(1);

await page.locator('[data-test-actions-toggle]').click();

// Test that the fieldset is not present.
await expect(page.locator('[data-test-placeholder-fieldset]')).toHaveCount(0);
await expect(page.locator('[data-test-version-yank-button="0.1.0"]')).toBeVisible();
Expand All @@ -106,6 +109,8 @@ test.describe('Acceptance | sudo', { tag: '@acceptance' }, () => {
await page.locator('[data-test-user-menu]').getByRole('button').click();
await page.getByRole('button', { name: 'Enable admin actions' }).click();

await page.locator('[data-test-actions-toggle]').click();

const yankButton = page.locator('[data-test-version-yank-button="0.1.0"]');
const unyankButton = page.locator('[data-test-version-unyank-button="0.1.0"]');

Expand Down
2 changes: 2 additions & 0 deletions e2e/acceptance/versions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ test.describe('Acceptance | crate versions page', { tag: '@acceptance' }, () =>
await expect(v020).not.toHaveClass(/.*latest/);
await expect(v020).not.toHaveClass(/.yanked/);

await v021.locator('[data-test-actions-toggle]').click();

// yanking
await page.locator('[data-test-version-yank-button="0.2.1"]').click();
await expect(v021).not.toHaveClass(/.*latest/);
Expand Down
3 changes: 3 additions & 0 deletions public/assets/ellipsis-circle.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions tests/acceptance/sudo-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ module('Acceptance | sudo', function (hooks) {
assert.dom('[data-test-disable-admin-actions]').doesNotExist();
assert.dom('[data-test-enable-admin-actions]').doesNotExist();

// Assert that there's no dropdown menu toggle, disabled, enabled, or in any state.
assert.dom('[data-test-actions-toggle]').doesNotExist();
// Assert that there's no yank button, disabled, enabled, or in any state.
assert.dom('[data-test-version-yank-button="0.1.0"]').doesNotExist();
});
Expand All @@ -62,6 +64,7 @@ module('Acceptance | sudo', function (hooks) {

// From the perspective of the actual button, it isn't disabled, even though
// the fieldset effectively makes it unclickable.
assert.dom('[data-test-actions-toggle]').exists();
assert.dom('[data-test-version-yank-button="0.1.0"]').exists();
});

Expand Down Expand Up @@ -90,6 +93,8 @@ module('Acceptance | sudo', function (hooks) {
}
assert.strictEqual(seen, 1);

await click('[data-test-actions-toggle]');

// Test that the fieldset is not present.
assert.dom('[data-test-placeholder-fieldset]').doesNotExist();
assert.dom('[data-test-version-yank-button="0.1.0"]').exists();
Expand All @@ -101,6 +106,8 @@ module('Acceptance | sudo', function (hooks) {
await visit('/crates/foo/versions');
await click('[data-test-enable-admin-actions]');

await click('[data-test-actions-toggle]');

await click('[data-test-version-yank-button="0.1.0"]');

await waitFor('[data-test-version-unyank-button="0.1.0"]');
Expand Down
2 changes: 2 additions & 0 deletions tests/acceptance/versions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ module('Acceptance | crate versions page', function (hooks) {
.hasNoClass(/.*latest/)
.hasNoClass(/.yanked/);

await click('[data-test-actions-toggle]');

// yanking
await click('[data-test-version-yank-button="0.2.1"]');
assert
Expand Down
Loading