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.

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

Fix search major bug #253

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Fix search major bug #253

merged 1 commit into from
Aug 21, 2024

Conversation

thePeras
Copy link
Member

There was a bug when I clear the storage and refreshing the page: I select the search major modal and start typing before it fetched the majors. It would break the app by calling some method on undefined. This way, the it returns before making something with the value.

I also have added the faculty to the option name, and refactoring some code.

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for tts-fe-preview ready!

Name Link
🔨 Latest commit 17bd785
🔍 Latest deploy log https://app.netlify.com/sites/tts-fe-preview/deploys/66c65573dcf03b000852f9cd
😎 Deploy Preview https://deploy-preview-253--tts-fe-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@tomaspalma tomaspalma left a comment

Choose a reason for hiding this comment

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

Great work 🚀 !

I just left a question about an optional suggestion that is not too important, so if you want to merge the PR as is, that is totally ok by me!

@@ -48,7 +45,7 @@ const MajorSearchCombobox = ({ selectedMajor, setSelectedMajor }: Props) => {
aria-expanded={open}
className="w-full justify-between dark:bg-darker dark:text-slate-50"
>
{selectedMajor ? majors.find((major) => major.id === selectedMajor.id)?.name : 'Seleciona um curso...'}
{selectedMajor ? selectedMajor.name : 'Seleciona um curso...'}
Copy link
Member

Choose a reason for hiding this comment

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

In this line do you think that we should also display the name of the faculty along the selectedMajor.name as it now also appears on the search results?

Copy link
Member Author

Choose a reason for hiding this comment

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

No need, since it also doesn't display the acronym. But if the team decides it could improve the experience, we can add it later

@thePeras thePeras merged commit 9b7ab57 into develop Aug 21, 2024
4 checks passed
@thePeras thePeras deleted the fix/search-major-bug branch August 21, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants