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

ftux: refactor + restructure + url rename #1281

Merged
merged 1 commit into from
May 29, 2018

Conversation

thelostone-mc
Copy link
Member

@thelostone-mc thelostone-mc commented May 26, 2018

Description

One step closer to achieving modularity

  • No flow specific templates are hardcoded in onboard.html
  • Specify flow in views -> and accordingly the flow will be built
  • All specific steps live it its own file
  • The CSS + JS is still common (will have to work on that )
  • the route is now updated to '/onboard/contributor' as against '/onboard'
  • All templates reside within ftux folder making it easier to maintain

@mbeacom Is this something along the lines of what you had in mind ?

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
  • routing
  • template rendering
Refers/Fixes

@thelostone-mc thelostone-mc requested a review from mbeacom May 26, 2018 06:26
@ghost ghost assigned thelostone-mc May 26, 2018
@ghost ghost added the in progress label May 26, 2018
@thelostone-mc thelostone-mc requested a review from SaptakS May 26, 2018 06:27
@codecov
Copy link

codecov bot commented May 26, 2018

Codecov Report

Merging #1281 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1281      +/-   ##
==========================================
- Coverage   31.26%   31.22%   -0.04%     
==========================================
  Files         120      120              
  Lines        8397     8397              
  Branches     1100     1100              
==========================================
- Hits         2625     2622       -3     
- Misses       5663     5666       +3     
  Partials      109      109
Impacted Files Coverage Δ
app/app/urls.py 94.28% <ø> (ø) ⬆️
app/dashboard/views.py 16.33% <0%> (ø) ⬆️
app/app/settings.py 83.61% <0%> (-1.7%) ⬇️

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 663124b...a596aa9. Read the comment docs.

@thelostone-mc thelostone-mc added frontend This needs frontend expertise. backend This needs backend expertise. and removed frontend This needs frontend expertise. labels May 26, 2018
Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Generally, lgtm so far. Just a few random comments.

</script>
<script>
var steps = [];
var flow = "{{flow}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

rabble rabble 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

It helps with the URL redirection :P

Copy link
Contributor

Choose a reason for hiding this comment

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

haha - I know. I wasn't talking about the flow and step args exactly xD {{flow}} 😁

return TemplateResponse(request, 'onboard.html', params)
params = {
'title': _('Onboarding Flow'),
'steps': ['github', 'metamask', 'avatar', 'skills'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much what I was thinking. Ideally, we'd take this a step ( 😁) further to allow us to provide the series of steps defined somewhere on the BE based on the user type or other criteria... Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty much what I was thinking.

Sauron is pleased :P

Couldn't we just add that logic in here ?
profile.getUserType() --> based on that we specify flow !
Any benefits your approach would offer over the one you laid out there ? (I probably didn't get this 😅 )

@thelostone-mc
Copy link
Member Author

@mbeacom good for merge ?

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm

@thelostone-mc thelostone-mc merged commit 2324f09 into gitcoinco:master May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This needs backend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants