Refactor thanks pages and a little bit of the self service page#331
Refactor thanks pages and a little bit of the self service page#331richardwestenra wants to merge 6 commits intovoteamerica:masterfrom
Conversation
richardwestenra
commented
Nov 4, 2017
richardwestenra
commented
- Fix a bug on the self-service page where the login form wasn't being hidden when you log in.
- Improve thanks page copy
- Remove unused assets
- Simplify the code and reduce repetition
|
@richardwestenra thanks for this 👍 overall it looks great, very useful changes 🎉 There are a couple of things I'm not sure about, and one that may break the automated tests (which I'm reluctant about so close |
|
|
||
| <div class="form-group"> | ||
| <button class="button button--danger" id="btnCancelRideRequest">Cancel Ride Request</button> | ||
| <button class="button button--danger" id="btnCancelRiderMatch">Cancel Rider Match Request</button> |
There was a problem hiding this comment.
This merging of the two cancel functions and buttons looks like a very promising refactor. I've probably not got time to check it before the Nov 7 election, and the changes to element ids may stop my automated tests from working. With the limited time and support team numbers, I'd like to keep those tests going if possible. Can we put this change to one side until after Nov 7, please?
There was a problem hiding this comment.
Sure, let's update the tests when we merge after Nov 7.
| <div class="wrapper single-column offset-top"> | ||
| <div class="bannerbox"> | ||
| <div class="bannerbox__content"> | ||
| <h1 id="thanks-header">Thank you</h1> |
There was a problem hiding this comment.
Can we keep the id for the h1 element until after Nov 7, please? The automated tests use it.
There was a problem hiding this comment.
Sure, let's update the tests when we merge after Nov 7.
| <div class="wrapper single-column offset-top"> | ||
| <div class="bannerbox"> | ||
| <div class="bannerbox__content"> | ||
| <h1 id="thanks-header">Congratulations</h1> |
There was a problem hiding this comment.
Can we keep the id for the h1 element until after Nov 7, please? The automated tests use it.
There was a problem hiding this comment.
Sure, let's update the tests when we merge after Nov 7.
| } | ||
|
|
||
| function createAPIurl (params, apiRoute) { | ||
| var url = ""; |
There was a problem hiding this comment.
I think this change might reflect our different coding styles. The tq lib is fantastic for changing the banners on the site for different partners, but I find when I come back to this code after a break, that it is slower for me to work out what is happening from the tq function name (set()) than reading some code. Usually, I have to find the library code and read that.
There was a problem hiding this comment.
Noted, thanks! I just felt it would help simplify things. The library is already included so I thought, why not use it? The documentation is here: https://github.com/richardwestenra/tiny-query-string#set - it's linked to from the file where the library is included.
I guess the method name isn't as clear as it could've been - what would you have called it?
There was a problem hiding this comment.
@richardwestenra This just feels like a coding style thing, to me, which is absolutely fine. My style, in the lack of a framework (not inherently an issue), is just to use the standard browser API throughout this page. For me, that API is widely documented and doesn’t give contributors anything new to learn. Still, there are at least two sides to that debate. My general thoughts are the code should suit whoever is mostly taking ownership of the page. If that will be you, great, go ahead and make use of any libraries that can help.
The name should match the behaviour if possible. I wasn’t sure if there were any side-effects to the lib function, and so issues arise if called twice before a call, etc?
jkbits1
left a comment
jkbits1
left a comment
There was a problem hiding this comment.
Hi @richardwestenra thanks for this. The changes to the thank you pages look great, and it is excellent to have another pair of eyes on the self-service page.
I've made some comments on specific lines, and everything else is good to go. What are your thoughts on my comments?
|
Thanks @jkbits1! I agree that we should hold off on merging until after the election. After that, can we update the automated tests? Also where are the tests? I couldn't find them. I would've thought it better to include front-end tests in this repository, unless that's not possible? Ultimately, I'd like to do more to simplify the jQuery on the self-service page. I find that at present, the code is very readable and easy to understand, but it's not as maintainable as I'd like, because of the duplication of concerns, so it's tricky to update because you need to update multiple places. I reckon we should try to avoid duplicate functions for drivers and riders, when the functionality is almost identical for each case. We should be able to differentiate between them with a few well-considered function parameters, I think? But this is a larger undertaking, and we should definitely wait until after the election. Maybe I'll make a new issue for it? What do you think? Thanks again for reviewing! :) |
|
@richardwestenra thanks for your patience 😺 I haven’t had time to respond to your comments before. In part, that’s because this PR addresses several concerns. It’s probably best if I reply piecemeal, otherwise there’ll always be too much to write in the time. So here’s something on the automated Travis CI tests: The location of the tests and other points are covered in our slack chat here: Now there is renewed interest in carpool-vote, I’ll take those comments and put them in the github repo for the testing or nightwatch folder (backend) when I can. The tests are functional, so they create an entire version of carpool-vote, including matching, and run tests against that. Because carpool-vote exists in two repos, there isn’t going to be a 100% clear-cut place to put them. Both development history and my own preference puts them in the back-end repo, though. It would be terrific to have you, and other front-end contributors, work on the tests. They can run automatically on PRs and merges to the front-end by including this file in the root folder: That file only exists in my repo at the moment. So, when I merged, earlier today, the latest front-end changes to my own repo, these tests were successfully run: tldr; it probably needs a bit of a joint effort to make the most of the automated tests, but much of the difficult work has been done, if we all consider them helpful. |
|
@richardwestenra at last, my final set of comments on this PR! 😄 How about you create a back-end PR that updates the tests for the changes here? From my info above, it shouldn’t be tough to do that. It’s probably best if you just take a guess at what seems right, rather than checking if the tests run. I’m more than happy to chip and work together on this. It would be a great way to get everyone involved with the tests, and should be a quick process. As an overview, any work on the Self-service page page needs some co-ordination and planning by whoever does that. Previous refactors have caused issues because they haven’t addressed how the entire API set works. Changes that look helpful can break other areas. The page has to support being used to support a list of APIs. I think anyone who follows that guideline can make good progress here. Generally, the self-service isn’t so hard as it might seem. In the lack of a framework, which is manageable, there is a basic concept of unidirectional code behaviour. The page is set up from the data provided in in the URL, then displayed. If the user clicks on something (login button, accept/cancel buttons), the page is setup from scratch and displayed again. This repeats indefinitely. Generally, I’m keen to merge this and deploy your improvements to the thanks pages. |
|
Thanks @jkbits1, I'll get onto this, uh, at some point. Currently pretty hectic in the run up to going away over xmas. |
|
Hey @jkbits1 I've tried to get the backend running on my computer but I don't know Docker and Postgres, and I found the repo instructions pretty confusing. I'm not sure I'll be able to figure it out without step-by-step directions :( |
2 participants
