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.

Sign up for GitHub

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

Jump to bottom

Replace Wikit Lookup component with Codex #773

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Replace Wikit Lookup component with Codex #773

merged 1 commit into from
Oct 24, 2024

Conversation

codders
Copy link
Contributor

@codders codders commented Oct 2, 2024 •
edited
Loading

Depends on changes to Codex: T376024 (Iddbd4a83a444b382d0b9318cd1831b42126464c4)

Replace usages of Wikit's Lookup compontent in ItemLookup and SpellingVariantInput with the CdxLookup component.

Bug: T370057

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 2, 2024

https://78835ae996ba6717368cfdb471e0f817268c--new-lexeme-special-page.netlify.app

@codders codders mentioned this pull request Oct 2, 2024
Closed
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch from da188e0 to fc0f367 Compare October 2, 2024 10:31
@codders codders marked this pull request as ready for review October 2, 2024 10:31
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch from fc0f367 to 94a80c3 Compare October 2, 2024 10:42
lucaswerkmeister
lucaswerkmeister reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@lucaswerkmeister lucaswerkmeister left a comment

Choose a reason for hiding this comment

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

The lookup styles look a bit spartan to me – the “no match was found” message doesn’t have any special styles, and the “label” of a match is also no longer bold. That’s not a blocker for merging, but I wouldn’t be surprised if it comes up during UX review.

src/styles/custom-variables.css Outdated Show resolved Hide resolved
src/components/ItemLookup.vue Outdated Show resolved Hide resolved
src/components/SpellingVariantInput.vue Outdated Show resolved Hide resolved
src/components/ItemLookup.vue Show resolved Hide resolved
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch 3 times, most recently from 4f68ec5 to 79f0a66 Compare October 8, 2024 07:20
@lucaswerkmeister
Copy link
Member

I’m trying to understand the CI errors. I assume the accessibility issues will be fixed with the next Codex release; but do we know what this other error means? ResizeObserver loop completed with undelivered notifications. sounds totally mysterious to me.

@codders
Copy link
Contributor Author

codders commented Oct 8, 2024

but do we know what this other error means? ResizeObserver loop completed with undelivered notifications. sounds totally mysterious to me.

I can only say that in my local testing with a modified codex, those errors go away. I'm assuming the error is a result of the cypress-axe code aborting harder than cypress otherwise expects.

@lucaswerkmeister
Copy link
Member

Alright, then I guess we’re just waiting here. I don’t have any other review comments – once CI passes I think this is good to go.

@lucaswerkmeister lucaswerkmeister mentioned this pull request Oct 11, 2024
Merged
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch from 79f0a66 to 646faa6 Compare October 17, 2024 07:26
@lucaswerkmeister
Copy link
Member

Hm, CI is unhappy :(

@codders
Copy link
Contributor Author

codders commented Oct 17, 2024

Yeah. The test suite is a bit frustrating. I took a quick look this morning, but I need a bit longer to dive back into how I made the tests pass last time.

@codders codders requested a review from yerdua October 24, 2024 09:46
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch from 646faa6 to 0cc4630 Compare October 24, 2024 09:47
@codders
Replace Wikit Lookup component with Codex
b8c614d 
Replace usages of Wikit's Lookup compontent in `ItemLookup` and
`SpellingVariantInput` with the `CdxLookup` component.

Bug: T370057
@codders codders force-pushed the feat/replace-wikit-lookup-component-20241002 branch from 0cc4630 to b8c614d Compare October 24, 2024 09:50
@codders
Copy link
Contributor Author

codders commented Oct 24, 2024

CI should now be happy again

yerdua
yerdua approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@yerdua yerdua left a comment

Choose a reason for hiding this comment

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

looks good to me, and CI does seem to be happy!

@yerdua yerdua merged commit b8a180d into main Oct 24, 2024
3 checks passed
@yerdua yerdua deleted the feat/replace-wikit-lookup-component-20241002 branch October 24, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaswerkmeister lucaswerkmeister lucaswerkmeister left review comments

@egardner egardner egardner left review comments

@yerdua yerdua yerdua approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codders @lucaswerkmeister @egardner @yerdua