Make the translation search bar consistent with other search bar #2032
Conversation
|
@njmulsqb is attempting to deploy a commit to the Quran Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I have fixed some of the eslint errors that caused the build to fail on vercel; locally eslint is showing many other suggestions in structure of code i.e. removing extra spaces, should I apply them? |
|
Assalam o Alaikum, can we merge this? |
| @@ -172,4 +174,5 @@ const TranslationSelectionBody = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line max-lines | |||
There was a problem hiding this comment.
Instead of doing that, we should split the file into smaller components.
There was a problem hiding this comment.
What additional component should I create and where? Just confirming if there's any specific pattern that you're following?
| id="translations-search" | ||
| value={searchQuery} | ||
| onChange={setSearchQuery} | ||
| onChange={(event) => { |
There was a problem hiding this comment.
This should be a separate function e.g. const handleOnSearchQueryChange than an inline one.
| @@ -25,6 +25,19 @@ | |||
| margin-block-end: var(--spacing-small); | |||
| } | |||
|
|
|||
| .searchInput { | |||
There was a problem hiding this comment.
This is a duplicate of .searchInput css inside src/components/Navbar/SearchDrawer/Header/Header.module.scss. While this is not a major issue but I think a much better approach is to create a new component called SearchInput for example inside our dls directory that can be reused by both the search drawer and translations search and any other component in the future. This way, will be sure they are 100% consistent and easier to update in the future.
Salamu Alikum brother, Jazak Allahu Kahiran for your contribution! I have left a couple of comments on your PR. Let me know if you need any further clarification |
|
@njmulsqb Could you please look review your PR following those recommendations @osamasayed gave so we can fix this issue. You are already close to the finish line. Kindly finish the good work you started. May Allah reward you abundantly. |
|
@deeseeker Would love to complete work on this, but I am getting following errors in the dev env, any idea how can they be resolved? |
Assalam o Alaikum, this is my first PR with Quran.com; if this gets merged I will be happy to work on more issues.
Summary
fixes #872
Screenshots