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 logic #75

Merged
merged 24 commits into from
Apr 21, 2020
Merged

Newsletter logic #75

merged 24 commits into from
Apr 21, 2020

Conversation

SantosGuillamot
Copy link
Member

@SantosGuillamot SantosGuillamot commented Apr 6, 2020

In order to test if the old logic was reusable I kind of migrated to the new web, in case it's useful for the final implementation. It works except some things I'm aware that should be improved:

  • Clean the code, right now it's a bit mess.
  • Finish the styles.

@SantosGuillamot SantosGuillamot linked an issue Apr 6, 2020 that may be closed by this pull request
@frontibotito
Copy link
Member

frontibotito commented Apr 6, 2020

Deploy preview for website ready!

Built with commit a483e4a

https://frontity-org-27ch6ysaz.now.sh

@SantosGuillamot SantosGuillamot mentioned this pull request Apr 6, 2020
I've left only the components in the components folder and moved actions
and state to the root. We don't use the name "store" anymore to avoid
introducing an additional concept, just actions and state.
Typings were declared but not being used.
The union was not working so I divided them into 2 different functions
@luisherranz
Copy link
Member

Nice work @SantosGuillamot!

I have refactored the code to help you structure things a little better. Also, typescript was right but it was not being applied. Take a look at the commits and commit messages and if you have any question let me know 👍

@michalczaplinski michalczaplinski self-assigned this Apr 14, 2020
@michalczaplinski
Copy link
Member

michalczaplinski commented Apr 14, 2020

@SantosGuillamot Leaving this in blocked for the moment as it's waiting on frontity/frontity#371 in order to send the correct events

The UI should be good to go if you want to review it though (it didn't require that many changes, actually🙂)

@SantosGuillamot
Copy link
Member Author

Looks great Michal! Some minor details that I'd implement if they aren't too difficult:

  • The current submit buttons have a weird border. I will delete it.
  • Once the input of the email is active, I'd add a border with frontity color to the whole box.
  • In the input of the afternewsletter, I'd use the same colors as in the one for the email.
  • I'd add some margin at the top for the mobile version.
  • In the current newsletter, the checkboxes and radio inputs are styled with Frontity colors, if it's easy I'd do something similar:

Checkbox
Screenshot from 2020-04-15 08-40-04

Radio input
Screenshot from 2020-04-15 08-40-31

@michalczaplinski michalczaplinski marked this pull request as ready for review April 21, 2020 22:51
@michalczaplinski michalczaplinski merged commit 8d1a1ba into dev Apr 21, 2020
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.

Newsletter
4 participants