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

Milestones module development #343

Open
wants to merge 121 commits into
base: master
Choose a base branch
from
Open

Milestones module development #343

wants to merge 121 commits into from

Conversation

phansier
Copy link

@phansier phansier commented Feb 8, 2018

Created:

  • List view of all repository milestones
  • Milestone view page
  • Milestone edit/create page
  • Dialog to add issue to milestone
  • Menu element to open milestones list
  • Milestones API support using Retofit

Made other changes related to Milestones

Changed some russian (ru) localisation strings.

arepina and others added 30 commits November 21, 2017 22:50
Added layouts
Added menu items
Added classes, activities, fragments
Remove redundant code
View include:
- title
- due to date
- description
- progress
- back to milestones list button
Copy link
Owner

@jonan jonan left a comment

Choose a reason for hiding this comment

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

I haven't tested everything yet, but I've added a few comments so you can start working on it if you want.

app/src/main/res/values-bg/strings.xml Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@
<string name="error_gists_load">Ошибка при загрузке Gists</string>
<string name="error_issue_load">Ошибка при загрузке задачи</string>
<string name="error_collaborators_load">Ошибка при загрузке соучастников</string>
<string name="error_milestones_load">Ошибка при загрузке целей</string>
Copy link
Owner

Choose a reason for hiding this comment

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

Don't update all the RU strings in this PR. You can send a new PR with this changes, but limit this one to just the new strings.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in phansier@e3a85c2
If OK, will be merged to PR

Moshi converter = new Moshi.Builder()
.add(new DateAdapter())
.add(Milestone.class, adapter)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need a custom adapter for milestones?

Copy link

Choose a reason for hiding this comment

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

To allow setting null due_date for milestone

Copy link
Owner

Choose a reason for hiding this comment

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

Can't you achieve the same result by modifying the current DateAdapter?

Copy link

Choose a reason for hiding this comment

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

No, default adapter for milestones just ignores null properties.
So there is no other way to send patch request with due_on = null.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

If we set due_on = null, then DateAdapter is not being called by Milestone adapter because it skips null properties.
Yes, we can achieve it only with DateAdapter, if use some date as null. For example, DateAdapter will replace 01/01/1990 to null. But I am not sure, that it is a better alternative.

assertFalse(filter1.equals(filter2));
filter2.setMilestone(milestone);
filter2.setMilestone(extraMilestone);
Copy link
Owner

Choose a reason for hiding this comment

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

This should be done removing the old org.eclipse.egit.github.core.Milestone code, simply creating the milestone with the new model object.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in phansier@9e5ae56
If OK, will be merged to PR

if (milestone != null){
issue.setMilestone(milestone.getOldModel());
}

Copy link
Owner

Choose a reason for hiding this comment

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

Whitespace missing before { 😉
And please remove last line break for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in phansier@30048f0
If OK, will be merged to PR

Call<List<Issue>> getIssues(
@Path("owner") String owner,
@Path("repo") String repo,
@Query("milestone") long milestone);
Copy link
Owner

Choose a reason for hiding this comment

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

More pagination problems.

Copy link
Author

Choose a reason for hiding this comment

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

We prefer to fix it in future because it is not the vital problem for milestones

<item
android:id="@+id/m_milestone"
app:showAsAction="never"
android:title="@string/milestone"/>
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably use the plural "Milestones" here.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in phansier@f493fe9
If OK, will be merged to PR

else
setGone(2, true);

setChecked(0, selected == position);
Copy link
Owner

Choose a reason for hiding this comment

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

Why delete this code?
You're breaking the issue filtering by milestone

Copy link
Author

Choose a reason for hiding this comment

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

Fixed together with UI fixes phansier@d3658ad
If OK, will be merged to PR

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.

6 participants