-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[BUG] Duplicate select form element #8147 #8220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
Please also update
- Storybook
stories/components/form/DropDown.stories.js
components/event/EventTabs.js
components/account/manage/navigation.js
components/Tabs.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eddiejaoude, I've fixed the errors you got. However, I don't know further tests that I can do because the Select component is only visible on a few pages, where the tests are now successful. Sorry for any inconvenience caused since I'm new to open-source development and still learning. I will now request a review again. |
components/form/Select.js
Outdated
name={name} | ||
className="mt-2 text-primary-high dark:bg-primary-high dark:text-white border-2 transition-all duration-250 ease-linear rounded px-6 py-2 mb-2 block w-full sm:text-sm sm:leading-6" | ||
defaultValue={value} | ||
id="dropdown-select" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think id
should be the same as name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eddiejaoude, sorry for the poor PRs made previously. I have finally made all tests and fixed the dropdowns wherever they are visible in the frontend. They all seem to be working now. I will now request for changes again. Sorry again for the delay and the inconvenience caused. |
The PR seems to have failed some tests, don't know exactly what they mean by the errors. If you want, I can try to look into them but I'd appreciate it if I get some help. |
The error message is
|
I have made the necessary changes and removed all conflicts, hopefully this time, the code will work fine. |
It failed one test again, I've been trying to run the tests locally however I get the error tests/setup/global-setup.js saying fetch is not defined. |
Make sure you are using Node 18 or newer. |
Hopefully the errors are now fixed as all tests run successfully locally. A change had to be made in the events test file. |
label: 'Test', | ||
options: options, | ||
handleEventTypeChange: (e) => console.log(e.target.value), | ||
onChange: (e) => console.log(e.target.value), | ||
className: '' | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DropDown.stories.js
file should be renamed to Select.stories.js
to match the component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you 👍
Thank you for giving me the opportunity Eddie! This was my first ever pull request that got merged and hopefully, I'll keep contributing to LinkFree!!! |
Fixes Issue #8147
Closes #8147
Changes proposed
[x] Deleted Dropdown.js form component
[x] Created a new Select form component including common and individual variables from both Dropdown and the previous Select component.
[x] Replaced the Dropdown component in userEvents.js with the new Select component.
[x] Fixed the Manage Event and Manage Milestone Typos.
Check List (Check all the applicable boxes)
Screenshots
No screenshots applicable as changes are only made to the code and there are no visible changes in the UI.
Note to reviewers
Sorry for closing the previous pull request since the naming convention of the variables was not up to the mark.