Skip to content

Conversation

@strazto
Copy link

@strazto strazto commented May 24, 2022

Description

Related Issue

Saw it discussed in #360

Screenshots

Types of changes

  • Breaking change (changes that break backward compatibility of public API or CLI - semver MAJOR)
  • New feature (non-breaking change which adds functionality - semver MINOR)
  • Bug fix (non-breaking change which fixes an issue - semver PATCH)

@strazto
Copy link
Author

strazto commented May 29, 2022

@jesec - Would you mind reviewing?

@strazto
Copy link
Author

strazto commented May 29, 2022

#414 (comment)

There is a minor issue: don't touch the translations. They are handled by Crowdin. Only change en.json.

I'm glad to hear that's the expectation, since that's all I did - do you think you could document this in the contributor guidelines?

@jesec
Copy link
Owner

jesec commented May 30, 2022

#414 (comment)

There is a minor issue: don't touch the translations. They are handled by Crowdin. Only change en.json.

I'm glad to hear that's the expectation, since that's all I did - do you think you could document this in the contributor guidelines?

I don't understand how that relates to this PR.

hash: <Hash />,
dateAdded: <Calendar />,
dateCreated: <CalendarCreated />,
dateFinished: <Calendar />,
Copy link
Owner

Choose a reason for hiding this comment

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

We would need a new icon here.

"torrents.properties.comment": "Comment",
"torrents.properties.date.added": "Added",
"torrents.properties.date.created": "Created",
"torrents.properties.date.finished": "Completed",
Copy link
Owner

Choose a reason for hiding this comment

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

Finished would be more consistent here.

{id: 'seeds', visible: true},
{id: 'dateAdded', visible: true},
{id: 'dateCreated', visible: false},
{id: 'dateFinished', visible: true},
Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to display this by default?

Copy link
Author

Choose a reason for hiding this comment

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

Up to you

Copy link
Author

Choose a reason for hiding this comment

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

I'm guessing you don't want it displayed by default, so I'll change it.

@strazto
Copy link
Author

strazto commented May 30, 2022

#414 (comment)

There is a minor issue: don't touch the translations. They are handled by Crowdin. Only change en.json.

I'm glad to hear that's the expectation, since that's all I did - do you think you could document this in the contributor guidelines?

I don't understand how that relates to this PR.

I found myself confused about what was expected of me for the translations, I found it on another PR, and I think it could be documented somewhere in some contributing guidelines

@jesec
Copy link
Owner

jesec commented Jul 11, 2022

I just realized that while the title said "Created", the code actually adds a "Finished" column.

A "Finished" column was added by #565, and we already have a "Created" column (at least for Condensed view).

@jesec jesec closed this Jul 11, 2022
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