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

Gitcoin Updates modal #5016

Merged
merged 8 commits into from
Sep 11, 2019
Merged

Gitcoin Updates modal #5016

merged 8 commits into from
Sep 11, 2019

Conversation

octavioamu
Copy link
Contributor

@octavioamu octavioamu commented Aug 14, 2019

Description

All the content is from a Gist file, so can be updated in any moment.
I created some rules to not affect performance or be annoying with the users:

  • just prompt for logged users
  • Don't fetch and open the modal if user had seen the modal in the last 7 days.
  • If the user had seen the modal more than 7 days ago, check if the content of the modal had changed, if is diff than the last time then open the modal.
  • Also I can bypass all the rules with a parameter if we want to link the modal in anywhere

image

Refers/Fixes

Fix #4896

Testing

https://embed.vidyard.com/share/DfE1gP7mvRvSh6e79DLNDF

@codecov
Copy link

codecov bot commented Aug 14, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5016   +/-   ##
=======================================
  Coverage   30.92%   30.92%           
=======================================
  Files         217      217           
  Lines       17402    17402           
  Branches     2381     2381           
=======================================
  Hits         5382     5382           
  Misses      11801    11801           
  Partials      219      219

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 1fa464a...e3a4c74. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 14, 2019

Codecov Report

Merging #5016 into master will increase coverage by 0.54%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5016      +/-   ##
==========================================
+ Coverage   30.93%   31.47%   +0.54%     
==========================================
  Files         217      218       +1     
  Lines       17457    18870    +1413     
  Branches     2392     2847     +455     
==========================================
+ Hits         5400     5939     +539     
- Misses      11836    12645     +809     
- Partials      221      286      +65
Impacted Files Coverage Δ
app/faucet/views.py 26.5% <0%> (-2.83%) ⬇️
app/marketing/views.py 11.7% <0%> (-0.19%) ⬇️
...cet/management/commands/process_faucet_requests.py 0% <0%> (ø)
app/app/settings.py 78.96% <0%> (+0.15%) ⬆️
app/retail/utils.py 11.01% <0%> (+0.77%) ⬆️
app/dashboard/views.py 15.49% <0%> (+1.48%) ⬆️
app/dashboard/router.py 36.05% <0%> (+1.53%) ⬆️
app/dashboard/models.py 58.59% <0%> (+1.56%) ⬆️
app/dashboard/utils.py 43.91% <0%> (+6.78%) ⬆️

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 106146e...4dbcae5. Read the comment docs.

Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

I think using a Gist for this is cool, but is there another place we can put it that's easier to manage? I just worry since theres no shared ownership for gists (I don't think...) so only @octavioamu can update it.

@octavioamu
Copy link
Contributor Author

@danlipert yes agree, there is not shared management of gists. Should we create it in our database instead? There is other external tool we can use for that? I was checking maybe gh wikis ?

@danlipert
Copy link
Contributor

danlipert commented Aug 21, 2019 via email

@thelostone-mc
Copy link
Member

Looks solid ! adding a don't merge label until we have content figured out to throw into the wiki ❤️

@octavioamu
Copy link
Contributor Author

looks like wiki are not available by gh API https://github.community/t5/How-to-use-Git-and-GitHub/API-for-repository-s-wiki/td-p/6706

@octavioamu
Copy link
Contributor Author

ok found a better way, now is an issue #5057

@danlipert
Copy link
Contributor

This good to merge today? What do y'all think? @octavioamu @thelostone-mc

@thelostone-mc thelostone-mc merged commit 9185ca3 into master Sep 11, 2019
@thelostone-mc thelostone-mc deleted the modal-updates branch June 27, 2020 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants