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

Add a "notes" section to the edit page #169

Merged
merged 8 commits into from
May 6, 2021
Merged

Add a "notes" section to the edit page #169

merged 8 commits into from
May 6, 2021

Conversation

theTisch21
Copy link
Member

This PR:
Adds a new property to the firebase document "notes"
Adds a parameter to the VALIDATE_FREE_FORM function to mark it optional, and accept null strings
Adds a field to the edit page where users can submit notes

@netlify
Copy link

netlify bot commented May 5, 2021

This link represents the deploy preview for this given pr.

Building with commit 2ddaf59

https://app.netlify.com/sites/ehs-service/deploys/60932bd5a4806a0007b16dae

@theTisch21 theTisch21 requested a review from davidvel25 May 5, 2021 23:18
@theTisch21 theTisch21 added the enhancement New feature or request label May 5, 2021
davidvel25
davidvel25 previously approved these changes May 5, 2021
Copy link
Collaborator

@davidvel25 davidvel25 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! If you have extra time (and know how to do it), you could make the text wrap around after it has hit the end of the bounding box, or you can implement a larger bounding box to be more square-like. Otherwise, it looks great!

Copy link
Member

@hazel-sudz hazel-sudz left a comment

Choose a reason for hiding this comment

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

@theTisch21 you need to rebase off of master and resolve conflicts: your files are behind by 2 commits

@theTisch21
Copy link
Member Author

@daniel-sudz
I'm not sure what happened, but it looks like it did something that hopefully deals with any merge conflicts.

@theTisch21 theTisch21 requested a review from hazel-sudz May 5, 2021 23:38
@Zjjc123
Copy link
Member

Zjjc123 commented May 6, 2021

@theTisch21 I'm giving you write access. Next time you can just make a new branch.

@hazel-sudz
Copy link
Member

hazel-sudz commented May 6, 2021

@theTisch21 I'm giving you write access. Next time you can just make a new branch.

@Zjjc123 that doesn't help with anything -- it's just a branch that was behind and not fast-forwarded before changes were made which results in merge conflicts. Where the branch is located (at origin or forked doesn't affect merging.)

@Zjjc123
Copy link
Member

Zjjc123 commented May 6, 2021

Oh this is unrelated to the issue. I just didn't know where to tell him that I gave him write

@theTisch21
Copy link
Member Author

@daniel-sudz @Zjjc123 I don't see the merge conflicts on my end. Would it be easier if I made a new branch based on master, cherry picked my commits over to the new branch, then made a new PR?

@theTisch21
Copy link
Member Author

@theTisch21 I'm giving you write access. Next time you can just make a new branch.

Thanks for write access!

Copy link
Member

@Zjjc123 Zjjc123 left a comment

Choose a reason for hiding this comment

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

We probably need to fix up the UI it's so ugly right now. But your changes look good to me but @daniel-sudz should check on it.

@Zjjc123
Copy link
Member

Zjjc123 commented May 6, 2021

@theTisch21 I don't see a merge issue. I think @daniel-sudz was talking about the issue before you fixed it.

@theTisch21
Copy link
Member Author

I don't see a merge issue either, and github says I can automatically merge, so I'm gonna do that. @daniel-sudz if you see a problem, you can undo this merge.

@theTisch21 theTisch21 merged commit 50322f1 into eastlakehs:master May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants