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

Newsletter switch #4448

Merged
merged 10 commits into from
Jun 24, 2019
Merged

Newsletter switch #4448

merged 10 commits into from
Jun 24, 2019

Conversation

octavioamu
Copy link
Contributor

@octavioamu octavioamu commented May 20, 2019

Description

image

Refers/Fixes

#4413

Testing

@octavioamu octavioamu changed the title Newsletter switch [WIP] Newsletter switch May 20, 2019
@octavioamu
Copy link
Contributor Author

octavioamu commented May 20, 2019

waiting new forms links
@frankchen07 @PixelantDesign

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #4448 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4448   +/-   ##
======================================
  Coverage    30.3%   30.3%           
======================================
  Files         214     214           
  Lines       17142   17142           
  Branches     2319    2319           
======================================
  Hits         5195    5195           
  Misses      11740   11740           
  Partials      207     207

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2098f26...02bfe4c. Read the comment docs.

@octavioamu octavioamu changed the title [WIP] Newsletter switch Newsletter switch Jun 20, 2019
@octavioamu octavioamu requested a review from a team June 20, 2019 15:55
@octavioamu
Copy link
Contributor Author

@gitcoinco/engineers Ready to review!!

@danlipert
Copy link
Contributor

@octavioamu looks good to me - how was it tested?

@thelostone-mc
Copy link
Member

^ @danlipert testing it is as simple as clicking on the tab and ensuring the subscribe button open the right form I believe!

cc @octavioamu

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@octavioamu looks like the styling hasn't been applied

Screenshot 2019-06-24 at 6 38 45 PM

Also, it redirects to bad page when I click on subscribe without entering name

http://consensys.us11.list-manage.com/subscribe/post?u=947c9b18fc27e0b00fc2ad055&id=d870d1315e

@octavioamu
Copy link
Contributor Author

octavioamu commented Jun 24, 2019

@thelostone-mc remember always to hard reload the browser to see the changes, here is a video https://embed.vidyard.com/share/1xuruC2LraV2ghEM482m2d?

@octavioamu
Copy link
Contributor Author

@thelostone-mc I had added validation

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

sweet

@octavioamu octavioamu merged commit 0955aff into master Jun 24, 2019
@thelostone-mc thelostone-mc deleted the newsletter-switch branch June 24, 2019 16:24
@danlipert
Copy link
Contributor

@thelostone-mc @octavioamu I ask just because the PR description didn't have any testing info - its a good way for us to double check as well as see what assumptions the PR creator is making during testing. Remember to hold off on merging until Aditya and I both review.

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.

3 participants