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

.has-danger form updates #3584

Merged
merged 16 commits into from Sep 11, 2019

Conversation

@youriwims
Copy link
Contributor

commented Aug 27, 2019

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3584 Aug 27, 2019 Inactive

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Aug 28, 2019 Inactive

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Aug 28, 2019 Inactive

@youriwims youriwims marked this pull request as ready for review Aug 28, 2019

@youriwims youriwims requested a review from natalieworth Aug 28, 2019

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Aug 28, 2019 Inactive

@natalieworth

This comment has been minimized.

Copy link

commented Aug 29, 2019

hey @youriwims the desktop looks good, but the mobile version is to the right of the content. I don't mind it though/ am okay with it if it's too difficult to add it beside the second line.
Screen Shot 2019-08-28 at 10 33 22 PM

@youriwims

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Thing is, when they're both in error form, it looks a bit off to me...idk. I think it's fine at the end 🤷‍♀

Screen Shot 2019-08-29 at 2 23 31 PM

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Aug 29, 2019 Inactive

@youriwims youriwims requested a review from kristinashu Sep 3, 2019

youriwims added 2 commits Sep 3, 2019

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3584 Sep 3, 2019 Inactive

@youriwims youriwims requested review from kristinashu and mmmavis and removed request for kristinashu Sep 3, 2019

@mmmavis

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@youriwims is this ready for code review or we are still waiting for @natalieworth to finish design review first?

@youriwims

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

No, this is ready for your review. I got design approval from Kristina. I'll remove Nat.

@youriwims youriwims removed the request for review from natalieworth Sep 3, 2019

@mmmavis

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@youriwims can you fix the base branch to it's development instead of master?

@youriwims

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Most def, I have it in my to-dos in my initial comment. Can we do that after code-review?

@mmmavis

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@youriwims um if the base branch is different we can't tell what the exact code changes we are adding the development. Is there something that's blocking you from changing the base branch now?

@youriwims youriwims changed the base branch from master to development Sep 3, 2019

@youriwims youriwims requested a review from mmmavis Sep 5, 2019

@mmmavis
Copy link
Member

left a comment

Don't forget to glance over code changes in the PR and remove irreverent irrelevant code before requesting review! 👀 (sorry for the typo)

Left some inline comments & found a text/icon overlap issue when email field is in error state.

image

source/js/components/join/join.jsx Outdated Show resolved Hide resolved
package-lock.json Show resolved Hide resolved
source/js/components/join/join.jsx Outdated Show resolved Hide resolved
source/js/components/join/join.jsx Outdated Show resolved Hide resolved
source/js/components/join/join.scss Outdated Show resolved Hide resolved
source/sass/glyphs.scss Outdated Show resolved Hide resolved
source/js/components/join/join.scss Outdated Show resolved Hide resolved
source/js/components/join/join.scss Outdated Show resolved Hide resolved
source/js/components/join/join.scss Outdated Show resolved Hide resolved

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Sep 6, 2019 Inactive

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Sep 6, 2019 Inactive

@youriwims

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@mmmavis I added the padding fix similar to the one you did for the Pulse search to fix the overlap. I also added comments to address two of your comments. Let me know if you have any ideas for the storing the validation function in the variable.

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Sep 6, 2019 Inactive

@youriwims youriwims requested a review from mmmavis Sep 6, 2019

@mmmavis
Copy link
Member

left a comment

Almost there!

source/js/components/join/join.jsx Show resolved Hide resolved
source/js/components/join/join.jsx Outdated Show resolved Hide resolved
source/js/components/join/join.jsx Outdated Show resolved Hide resolved

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Sep 10, 2019 Inactive

@youriwims youriwims requested a review from mmmavis Sep 10, 2019

@mmmavis

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@youriwims code looks good but since development branch has been merged into master, you will have to rebase your branch (again 😬) off of master. I'll do a quick review after the rebase just to be safe.

@youriwims

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

ah yes!doing that now!

@youriwims youriwims changed the base branch from development to master Sep 10, 2019

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-3584 Sep 10, 2019 Inactive

source/sass/glyphs.scss Outdated Show resolved Hide resolved
source/sass/glyphs.scss Outdated Show resolved Hide resolved
@mmmavis
Copy link
Member

left a comment

Looks awesome! Nice work I left some comments regarding changing some file names so they follow our naming convension. Feel free to just merge this PR after you update those names.

@alanmoo alanmoo merged commit 4465b87 into master Sep 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/foundation.mozilla.org Visual review approved by Youri
Details

@alanmoo alanmoo deleted the issue-3508-fix branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.