Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

[BUG] Duplicate select form element #8147 #8220

Merged
merged 32 commits into from
Aug 6, 2023

Conversation

mianakbarjan
Copy link
Contributor

@mianakbarjan mianakbarjan commented Jul 16, 2023

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)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

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.

@github-actions github-actions bot added large Pull request with more than 30 changed lines waiting-for-reviewers labels Jul 16, 2023
Copy link
Member

@eddiejaoude eddiejaoude left a 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

@SaraJaoude SaraJaoude added the issue linked Pull Request has issue linked label Jul 17, 2023
Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution but I seem to get many errors ...

EVENTS page

Screenshot 2023-07-18 at 07 56 32

ACCOUNT STATISTICS page

Screenshot 2023-07-18 at 07 57 02

Please test your changes before requesting a review

@mianakbarjan
Copy link
Contributor Author

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.

@mianakbarjan mianakbarjan requested a review from eddiejaoude July 19, 2023 19:47
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"
Copy link
Member

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

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes.

Some of the dropdowns don't work though...

Screenshot 2023-07-23 at 15 05 17

@mianakbarjan
Copy link
Contributor Author

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.

@mianakbarjan mianakbarjan requested a review from eddiejaoude July 24, 2023 20:38
@mianakbarjan
Copy link
Contributor Author

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.

@eddiejaoude
Copy link
Member

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 Ensures select element has an accessible name with solutions...

  • "message": "Form element does not have an implicit (wrapped) ",
  • "message": "Form element does not have an explicit ",
  • "message": "aria-label attribute does not exist or is empty",
  • "message": "aria-labelledby attribute does not exist, references elements that do not exist or references elements that are - "message": "Element has no title attribute",
  • "message": "Element's default semantics were not overridden with role="none" or role="presentation"",

Fix any of the following:
+ Form element does not have an implicit (wrapped)
+ Form element does not have an explicit
+ aria-label attribute does not exist or is empty
+ aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty
+ Element has no title attribute

@mianakbarjan
Copy link
Contributor Author

I have made the necessary changes and removed all conflicts, hopefully this time, the code will work fine.

@mianakbarjan
Copy link
Contributor Author

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.

@dan-mba
Copy link
Member

dan-mba commented Aug 4, 2023

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.
Fetch was added in node 18.

@github-actions github-actions bot added the tests label Aug 5, 2023
@mianakbarjan
Copy link
Contributor Author

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: ''
}
};
Copy link
Member

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

components/Tabs.js Outdated Show resolved Hide resolved
components/Tabs.js Outdated Show resolved Hide resolved
Copy link
Member

@eddiejaoude eddiejaoude 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, thank you 👍

@eddiejaoude eddiejaoude merged commit b1c5731 into EddieHubCommunity:main Aug 6, 2023
@mianakbarjan
Copy link
Contributor Author

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!!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked large Pull request with more than 30 changed lines storybook tests waiting-for-reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Duplicate select form element
4 participants