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 #194: Simplify role-changing machinery with json and js #225

Merged
merged 6 commits into from
Mar 20, 2018

Conversation

apooravc
Copy link
Contributor

@apooravc apooravc commented Feb 20, 2018

Fixes #194

  • Deleted roles.txt and update_roles.py.

  • Added roles.json, an equivalent of roles.txt.

  • Used some JS to populate roles table from the json file.

  • Added roles descriptions to json file and created those roles lists from it.

json entry format
{
"role" : "role name",
"url" : "div element url",
"sub-role" : ["sub-role-1", "sub-role-2", "sub-role-3"],
"lead" : ["lead-1", "lead-2", "lead-3"],
"deputy" : ["deputy-1", "deputy-2", "deputy-3"],
"role-head" : ["role heading"],
"strong" : [["strong-title-1"], ["strong-title-2"]],
"sub-description" : [["sub-description-1"], ["sub-description-2"]],
"description" : ["list-item-1", "list-item-2", "list-item-3"]
}

One way to see the changes locally would be as follows:

  1. Clone my repo from https://github.com/apooravc/astropy.github.com
  2. After cloning, only the master branch would be available locally. Enter git checkout -b simplify-role-changing origin/simplify-role-changing to get the branch simplify-role-changing locally.
  3. The changes could then be seen on team page.

@eteq Please have a look.

@bsipocz bsipocz requested a review from eteq February 20, 2018 12:20
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

@apooravc - this is great! Thanks for taking the time to work on this. Very close to exactly what I was hoping for. I have one inline comment (see below) and one broader item:

As you've implemented it, all of the information to build the table is in the roles.json file, which is great! However, in the description section of the site still requires some changes on the HTML side: we add new roles fairly regularly, so it would be better if instead no modification to the HTML was required at all. Basically, this would mean removing all of the sections that in your version have a <br/>, <hr>...</h3>, <p>...</p>, and the <ul class="roles-list"></ul>, and instead having them be generated from javascript (using the kind of DOM manipulation you're already doing to make the roles-list). The only content that I think you have to move, then, is the part of the description that's currently inside the <p> tags.

Make sense?

(If you care about these sort of things, this is a classic example of a Model-View-Controller situation: the Model is the JSON file, the View is the HTML, and the Controller is the js file. In the current implementation, a little bit of the model is in the HTML, and I'm suggesting we move it entirely out into the JSON)

js/functions.js Outdated
});
$(".roles-list").eq(index).append(list);
} else {
//separate case for index 5 representing tutorials & guides section
Copy link
Member

Choose a reason for hiding this comment

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

Maybe modify this to not be specific to a particular index? E.g., instead of the if statement being if (index != 5) have it be if (role["description"].length == 2 & role["description"][0] instanceof Array)? (if you use that if you'll have to swap the if and else blocks, but hopefully you get the idea?)

@eteq
Copy link
Member

eteq commented Mar 9, 2018

(One other minor suggestion for the future @apooravc : usually it's best to have commit messages be more specific than "fix". For example, instead of "Third fix for #194" you might say "added urls to javascript-generated table". No change needed, but just something to keep in mind for the future.)

@apooravc
Copy link
Contributor Author

@eteq Did the required changes, please have a look.

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

Thanks @apooravc! I think it looks great, but I saw just a couple minor things that might make things a bit easier to maintain down the road. But ones those are fixed I think we are all set!

js/functions.js Outdated
//index marks current lead
var index = 0;
//regular expression used in searching for 1 in a string
var regExp = /^1/i;
Copy link
Member

Choose a reason for hiding this comment

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

this regExp variable could have a slightly clearer name - I was trying to understand and it took me a while to realize it was the "deputy" part. Maybe call it regExpPreferDeputy or similar?

roles.json Outdated
"deputy" : [],
"role-head" : ["Coordination committee member (4 people)"],
"strong" : [],
"sub-description" : ["Overall coordination and management of the Astropy project, including:"],
Copy link
Member

Choose a reason for hiding this comment

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

I think the description and sub-description or swapped here. What you have as sub-description seem to be the more minor items, whereas description are the big things. Maybe just swap the two?

@eteq
Copy link
Member

eteq commented Mar 20, 2018

With a day to think I realized I could just do those minor changes myself. So this is all set and I will merge it shortly. Thanks @apooravc !

@eteq eteq merged commit 4d5d5d3 into astropy:master Mar 20, 2018
@apooravc apooravc deleted the simplify-role-changing branch March 21, 2018 07:54
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.

2 participants