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

Manually approve pages in QA review #1576

Merged
merged 9 commits into from Mar 12, 2024
Merged

Manually approve pages in QA review #1576

merged 9 commits into from Mar 12, 2024

Conversation

SuaYoo
Copy link
Collaborator

@SuaYoo SuaYoo commented Mar 5, 2024

Partially addresses #1477

Changes

  • Automatically update view to first page if page ID isn't specified
  • Show current page URL in location bar (resolves QA UI: Page location bar #1495)
  • Approve, reject, or leave notes on a page
  • Display temporary list of links to pages in the sidebar

You can access this view by adding /review/screenshots to a crawl detail page URL. The page IDs on the side are clickable.

Manual testing

Testing steps in #1534

Screenshots

Page Image/video
QA Review Screenshot 2024-03-05 at 10 50 48 AM
QA Review Screenshot 2024-03-11 at 3 32 21 PM

@SuaYoo SuaYoo requested a review from emma-sg March 5, 2024 18:54
@SuaYoo SuaYoo marked this pull request as ready for review March 5, 2024 18:55
@SuaYoo SuaYoo marked this pull request as draft March 11, 2024 21:29
@SuaYoo SuaYoo changed the title wip: Fetch crawl pages in QA review Fetch crawl pages and set default page in QA review Mar 11, 2024
@SuaYoo SuaYoo marked this pull request as ready for review March 11, 2024 23:05
Copy link
Member

@emma-sg emma-sg left a comment

Choose a reason for hiding this comment

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

The toolbar doesn't seem to do well with longer URLs/smaller viewport sizes. This should probably be clipped with an ellipse? Left some suggestions that should fix this.
image

Also a couple linting issues — should be most auto-fixable, I think.

])}
</div>
<div
class="mx-1.5 flex h-8 flex-1 items-center justify-between rounded border bg-neutral-0 px-2 text-sm leading-none"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class="mx-1.5 flex h-8 flex-1 items-center justify-between rounded border bg-neutral-0 px-2 text-sm leading-none"
class="mx-1.5 flex h-8 min-w-0 flex-1 items-center justify-between rounded border bg-neutral-0 px-2 text-sm leading-none"

<div
class="mx-1.5 flex h-8 flex-1 items-center justify-between rounded border bg-neutral-0 px-2 text-sm leading-none"
>
<span>${this.page?.url || "http://"}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<span>${this.page?.url || "http://"}</span>
<span class="flex-1 truncate">${this.page?.url || "http://"}</span>

Copy link
Collaborator Author

@SuaYoo SuaYoo Mar 12, 2024

Choose a reason for hiding this comment

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

Do we want to truncate the URL if it's how the current page is identified? Maybe have it scrollable instead?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's a good point — should it be focusable, and switch to being scrollable when it's focused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to scroll!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also should've noted that we'll probably revisit the location bar anyway in the subtasks, this is to add one as a starting point.

@@ -118,43 +156,144 @@ export class ArchivedItemQA extends TailwindElement {
<h1>${msg("Review")} &mdash; ${itemName}</h1>
</header>
<section class="main outline">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<section class="main outline">
<section class="main min-w-0 outline">

@SuaYoo SuaYoo requested a review from emma-sg March 12, 2024 01:37
Adds button group to QA review page for approving, rejecting, and adding
comments to a page.

---------

Co-authored-by: Henry Wilkinson <henry@wilkinson.graphics>
@SuaYoo SuaYoo changed the title Fetch crawl pages and set default page in QA review Manually approve pages in QA review Mar 12, 2024
@SuaYoo SuaYoo merged commit 9f312c0 into main Mar 12, 2024
2 checks passed
@SuaYoo SuaYoo deleted the frontend-qa-pages branch March 12, 2024 17:08
@Shrinks99 Shrinks99 mentioned this pull request Mar 12, 2024
1 task
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.

QA UI: Page location bar
2 participants