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

Linked About page #13

Closed
wants to merge 2 commits into from
Closed

Linked About page #13

wants to merge 2 commits into from

Conversation

reubenvarghese1
Copy link
Collaborator

Linked about page and also added use information as planned

@@ -3833,6 +3833,13 @@
}

]
},
{
Copy link
Member

Choose a reason for hiding this comment

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

Don't put this here. We should not hackily add a "list" that is not actually a list.

@@ -14,7 +14,7 @@ export default class ListOfLists extends Component {
}

render() {
const LISTS_TO_SHOW = 3;
const LISTS_TO_SHOW = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Revert this

@@ -25,7 +25,7 @@ export default class ListOfLists extends Component {
<ul className="nav nav-pills">
{toShow.map(list =>
<li className={list.uid === selectedUid ? "active" : ""}>
<Link to={`/list/${list.uid}`}>
<Link to={list.name.toString() === "About" ? `/About` : `/list/${list.uid}`}>
Copy link
Member

Choose a reason for hiding this comment

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

Don't do this. It makes the code look hacky to special-case this.
Ideally we'd add the About link outside of this component (since it doesn't strictly belong in ListOfLists), but it's okay to just put another li element after the More Lists thing. E.g. it's okay to have:

  • List 1
  • List 2
  • List 3
  • ...
  • About

a) Bintray <br/>
b) Modulus <br/>
<br/>
Currently we only support these 2 servers but in the future we sure will add more based on user demand! Currently, we are working on adding support for Github Releases as well!
Copy link
Member

Choose a reason for hiding this comment

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

This line is too much information. :-)

Instead, in the contributing section below, let's add a link like "Discuss how this tool works and/or propose new features at (link)". And that should link to the new Talk category that I'm suggesting at https://talk.openmrs.org/t/create-a-category-for-the-add-on-index-project/11700

<p> Adding your module to Add Ons for indexing is as easy as creating a pull request!
Before we do that, we need to ensure that your module has been hosted on one of the sites that Add Ons currently supports indexing from: <br/>
a) Bintray <br/>
b) Modulus <br/>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of Modulus, call this "modules.openmrs.org".
(Technically Modulus is the name of the underlying software, not the tool we have running.)

<a target="_blank"
href="https://github.com/djazayeri/openmrs-contrib-addonindex/blob/master/README.md">Read more on
GitHub</a>
<h3>2)How to contribute to Add Ons development?</h3>
Copy link
Member

Choose a reason for hiding this comment

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

I suggest rephrasing this as "About the OpenMRS Add Ons tool"

It can have links to:

</div>

<h3>Addons Stats:</h3>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

For consistency:

  1. This should be <p> instead of <div> like in the other two sections.
  2. For the other two sections, get rid of the 1) and 2)

@@ -121,8 +121,10 @@ public void testModuleBackendDetails() throws Exception {
@Test
public void testListsHaveNames() throws Exception {
for (AddOnList list : toIndex.getLists()) {
if (!list.getName().toString().equals("About")){
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this, this is hacky. :-)

@reubenvarghese1
Copy link
Collaborator Author

Closing this PR and creating a new one which links to Jira

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