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

Fix bug 1468997: Request a new locale directly through /teams page. #1133

Merged
merged 11 commits into from
Nov 29, 2018

Conversation

vishalol
Copy link
Collaborator

@vishalol vishalol commented Nov 20, 2018

The UI is kept same as requesting a project from /team page. The request is mailed to project managers.

image

pontoon/teams/views.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Nice work @vishalCR7!

I have three requests in addition to the nits commented in the bug:

  1. You need to fix the JS error that pops up when you try to make a request:
TypeError:
Pontoon.requestProjects is undefined; can't access its "toggleButton" property
request_locales.js:63:13
  1. Styling of the request form should match the /settings/ page. Mind the field style (background, border, radius, label colors, font size...) and drop columns in the labels. Also note that the request button should be painted yellow when in "Are you sure?" state.

  2. You copy-pasted lots of code from the "request a project" functionality. That's not a good idea, because when we'll want to make a change, we'll have to make it in two places now. I suggest we join request_projects anf request_locale into a single view with an optional locale parameter. And refactor the static code a little to have a shared request.css and request.js file with contents of team.css and teams.css that is used in the request projects/team forms. We can also postpone the static files refactor until https://bugzilla.mozilla.org/show_bug.cgi?id=1435179, but only under the condition that you're ready to jump on it after this bug is fixed. :)

pontoon/teams/static/css/teams.css Outdated Show resolved Hide resolved
pontoon/teams/static/css/teams.css Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/widgets/team_list.html Outdated Show resolved Hide resolved
pontoon/teams/static/js/request_locales.js Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/widgets/team_list.html Outdated Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/teams.html Outdated Show resolved Hide resolved
pontoon/teams/urls.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Nice work! There are 3 open items left (the button should read "Request new team", if the team already exists we should report that on frontend, full size search bar for non-authenticated users) from the first review and I added a few more. Nothing serious though, we're getting there!

pontoon/teams/urls.py Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/widgets/team_list.html Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Really great work, @vishalCR7! Everything works as exptect and all comments have been addressed.

Sorry for adding more. They are mostly code-style nits, but since you're an active contributor to Pontoon, you have to learn them at some point. :) I should have added those comments earlier, but I wanted the PR to work as expected first.

pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/css/request.css Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/request.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

I added a few smaller changes in the final commit, because it's easier that way than via comments. I hope you are OK with that.

Really good work with the PR, looking forward to hacking together soon! :)

@mathjazz mathjazz merged commit b4dee05 into mozilla:master Nov 29, 2018
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