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

Added invalid dates warning and out of range date warning #101

Closed
wants to merge 0 commits into from

Conversation

AviadCohen24
Copy link

Added a warning to select date that inside the dates range. the selected range for now it's from 01-01-2010 until every day
image

Added a warning for invalid dates.
image

Close #80

@NoamGaash NoamGaash self-requested a review October 7, 2023 11:10
Copy link
Member

@NoamGaash NoamGaash 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! It looks great.
Would you like to fix my suggestion or merge it as-is?
🙌

src/pages/dashboard/DashboardPage.tsx Outdated Show resolved Hide resolved
@AviadCohen24
Copy link
Author

Yes of course, I will fix it and push the changes

@NoamGaash
Copy link
Member

thank you! 🥇 ⭐
It looks so mush better :)

@NoamGaash
Copy link
Member

let's fix the build and merge it
💪
image

@AviadCohen24
Copy link
Author

AviadCohen24 commented Oct 7, 2023

ok, I'm on it

@NoamGaash
Copy link
Member

@AviadCohen24 do you need any help?

@AviadCohen24
Copy link
Author

Well, I pushed again with some fixes that you suggest, and I have to say that even that i didn`t add a lot of code lines in that PR, i learned how to be more "cleaner" in my code, for example- the warning selector, at the start I though about using function that handle different error states, but because of this PR I learned that i can use an object and indexes (like you suggested) and make it more simple than using function

@NoamGaash
Copy link
Member

@AviadCohen24 thank you! how exciting it is to hear that.
please try to resolve the conflicts by yourself, if it gets too messy tell me and I'll help

@AviadCohen24
Copy link
Author

@NoamGaash Ok, I`m on it

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

looks great, thanks!

@NoamGaash
Copy link
Member

you'll have to fix the build before merging

Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for astonishing-pothos-5f81bd ready!

Name Link
🔨 Latest commit c8321a2
🔍 Latest deploy log https://app.netlify.com/sites/astonishing-pothos-5f81bd/deploys/6559a5d586484c0008eccf98
😎 Deploy Preview https://deploy-preview-101--astonishing-pothos-5f81bd.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.

@shootermv
Copy link
Collaborator

the reason for merge conflicts now - is that instead of DatePicker mui component, dashboard page is now uses DateSelector component (which is our extension for mui DatePicker)

@shootermv
Copy link
Collaborator

i wonder, what is the status here, @AviadCohen24 are you still work on it?

@AviadCohen24
Copy link
Author

@shootermv Sorry, but unfortunately I'm in a busy period with the situation in Israel and I don't have time to work on it. If someone else can help me with this and take care of what is needed I would greatly appreciate it

@shootermv
Copy link
Collaborator

No prob,
You did very good job!

@ArkadiK94
Copy link
Collaborator

@shootermv Hi, why did you close this pr?

@shootermv
Copy link
Collaborator

shootermv commented Nov 19, 2023 via email

@shootermv
Copy link
Collaborator

shootermv commented Nov 19, 2023 via email

@NoamGaash
Copy link
Member

Hi @AviadCohen24
If possible, please run git push -f in your local branch.
that should fix this PR
thanks 🙏

@AviadCohen24
Copy link
Author

Of course, tomorrow I'll finally have time to do it. Sorry about the delay 😅

@AviadCohen24
Copy link
Author

Of course, tomorrow I'll finally have time to do it. Sorry about the delay 😅

I did :)

@shootermv
Copy link
Collaborator

I have your code saved at my local machine if you need...

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.

DatePicker component | show invalid dates warning
4 participants