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

Create Search component #63

Merged
merged 8 commits into from
Sep 28, 2023
Merged

Create Search component #63

merged 8 commits into from
Sep 28, 2023

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Aug 23, 2023 •
edited
Loading

@germain-gg germain-gg requested a review from a team as a code owner August 23, 2023 12:59
@germain-gg germain-gg mentioned this pull request Aug 23, 2023
Open
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 23, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d2646e1
Status: ✅  Deploy successful!
Preview URL: https://18188006.compound-web.pages.dev
Branch Preview URL: https://germain-gg-search.compound-web.pages.dev

View logs

weeman1337
weeman1337 suggested changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Some notes:

  • The disabled state is not really discoverable (It looks the same than enabled). But it is also not in design. Is this going to change?
  • The icon is smaller than in design

image

src/components/Search/Search.tsx Show resolved Hide resolved
src/components/Search/Search.tsx Show resolved Hide resolved
src/components/Search/Search.module.css Outdated Show resolved Hide resolved
src/components/Search/Search.module.css Outdated Show resolved Hide resolved
@germain-gg germain-gg marked this pull request as draft August 31, 2023 20:32
@germain-gg
Copy link
Contributor Author

Back in draft for now as matrix-org/matrix-react-sdk#11507 will alleviate the concern and need for that component in the short term

@germain-gg germain-gg removed the request for review from americanrefugee August 31, 2023 20:33
@germain-gg
Copy link
Contributor Author

The is no disabled state for now, henced removed it from the props the component receives. Might be added at a later stage.

@germain-gg germain-gg marked this pull request as ready for review September 27, 2023 09:16
weeman1337

This comment was marked as outdated.

Sign in to view

@weeman1337
Copy link
Contributor

Also dark theme doesn't look so good

image

@germain-gg
Copy link
Contributor Author

The placeholder is set correctly, you can validate it by checking the shadow dom element as explained on https://stackoverflow.com/questions/26852922/inspect-webkit-input-placeholder-with-developer-tools

weeman1337
weeman1337 approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 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 👍

@germain-gg germain-gg merged commit 3a27419 into main Sep 28, 2023
6 of 7 checks passed
@germain-gg germain-gg deleted the germain-gg/search branch September 28, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weeman1337 weeman1337 weeman1337 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@germain-gg @weeman1337