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

Supplemental file add/remove UI #4080

Merged
merged 5 commits into from
Apr 14, 2020
Merged

Supplemental file add/remove UI #4080

merged 5 commits into from
Apr 14, 2020

Conversation

Dananji
Copy link
Contributor

@Dananji Dananji commented Apr 9, 2020

Screenshot from 2020-04-13 14-38-54

@Dananji Dananji changed the title Transcript ui [WIP] Transcript ui Apr 9, 2020
@Dananji Dananji changed the title [WIP] Transcript ui Supplemental file add/remove UI Apr 13, 2020
@Dananji Dananji marked this pull request as ready for review April 13, 2020 18:40
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

👍 This looks good!
I had a few questions/suggestions for cleanup.

@@ -211,6 +255,13 @@ Unless required by applicable law or agreed to in writing, software distributed
$('#' + target).toggleClass('hidden');
});

$('.files_toggle').on('click', function (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this one line be unindented? It looks odd as it is right now.

Comment on lines 605 to 607
div.transcript_file_view {
display: flex;
}
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this is used. Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch 👍

inputField.focus();
});

$('button[name="label_edit_cancel"]').on('click', e => {
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 would it make sense to rename this button cancel_edit_label?

Comment on lines +30 to +31
// Page reload to show the flash message
location.reload();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it is really necessary to reload the page for a label update. Maybe an inline indication of success would be better? But I'd wait on feedback from PO before changing this since it looks like it already works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that sounds good to me. Now that I think about it, some inline edit forms do not have feedback flash messages (e.g. editing markers in playlists, add to playlist in item page. these are the inline forms I can think of right now).

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

I think this is ready now but I think it makes sense for it to wait to be merged until the model/controller PR has been merged (which will hopefully be soon). Is that okay with you?

@Dananji
Copy link
Contributor Author

Dananji commented Apr 13, 2020

I think this is ready now but I think it makes sense for it to wait to be merged until the model/controller PR has been merged (which will hopefully be soon). Is that okay with you?

Yes I agree 💯

@cjcolvar cjcolvar merged commit 78d938f into develop Apr 14, 2020
@cjcolvar cjcolvar deleted the transcript_ui branch April 14, 2020 13:21
@joncameron joncameron mentioned this pull request Jun 9, 2020
22 tasks
@Dananji Dananji mentioned this pull request Oct 14, 2020
22 tasks
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