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

[DONE] Check extesion allowed video filename #1033

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

marchal-julien
Copy link
Member

@marchal-julien marchal-julien commented Jan 29, 2024

Before sending your pull request, make sure the following are done :

  • You have read our contribution guidelines.
  • Your PR targets the develop branch.
  • The title of your PR starts with [WIP] or [DONE].

@marchal-julien marchal-julien self-assigned this Jan 29, 2024
@ptitloup ptitloup added the bug Something isn't working label Jan 29, 2024
@marchal-julien marchal-julien changed the title Check extesion allowed video filename [DONE] Check extesion allowed video filename Jan 29, 2024
Copy link
Contributor

@ptitloup ptitloup left a comment

Choose a reason for hiding this comment

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

merci pour la contribution !
demande de modification : amélioration de l'indentation, retrait de jquery.

data.submit();
var allow_extension_array = this.accept.replaceAll(' ', '').split(",");
var extension = data.originalFiles[0].name.substr((data.originalFiles[0].name.lastIndexOf('.') +1)).toLowerCase();
if ($.inArray("."+extension,allow_extension_array) == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$ fonction jquery qui ne doit plus etre utilisé ! :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

à priori ceci devrait être équivalent :

if (allow_extension_array.includes("."+extension) == false) {

Copy link
Member Author

Choose a reason for hiding this comment

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

solve (merci pour l'aide)

pod/video/templates/videos/add_video.html Outdated Show resolved Hide resolved
pod/video/templates/videos/add_video.html Outdated Show resolved Hide resolved
pod/video/templates/videos/add_video.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@Badatos Badatos left a comment

Choose a reason for hiding this comment

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

en gros, même remarques que Nicolas : tant qu'on peut éviter Jquery c'est mieux ;)

data.submit();
var allow_extension_array = this.accept.replaceAll(' ', '').split(",");
var extension = data.originalFiles[0].name.substr((data.originalFiles[0].name.lastIndexOf('.') +1)).toLowerCase();
if ($.inArray("."+extension,allow_extension_array) == -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

à priori ceci devrait être équivalent :

if (allow_extension_array.includes("."+extension) == false) {

Remove jquery part
Copy link
Contributor

@ptitloup ptitloup left a comment

Choose a reason for hiding this comment

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

ok pour moi, merci !

}
var elementsShowProcessing = document.getElementsByClassName("show-on-processing");
for (let i = 0; i < elementsShowProcessing.length; i++) {
elementsShowProcessing[i].style.display = 'block';
Copy link
Collaborator

Choose a reason for hiding this comment

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

ici, il est préférable de retirer la classe "d-none" plutot que d'ajouter un style inline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nous avons regardé mais ces elements n'ont pas la classe d-none ! Ils ont juste un style display=none

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je ne comprend pas : le d-none vient juste d'etre ajouté 2 lignes plus haut o_O

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Ligne 178)

Copy link
Contributor

Choose a reason for hiding this comment

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

ce ne sont pas les mêmes éléments ! (show-on-processing et hide-on-processing)

Copy link
Collaborator

@Badatos Badatos Jan 30, 2024

Choose a reason for hiding this comment

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

ils devaient bien avoir un display block avec JQ, mais maintenant qu'on a corrigé, y'a pas de raison

Copy link
Contributor

Choose a reason for hiding this comment

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

ok pour toi ? au pire on merge et on améliore mais on pourra pas retirer jquery de cette page car on en a besoin pour le chuncked upload

Copy link
Collaborator

Choose a reason for hiding this comment

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

ce ne sont pas les mêmes éléments ! (show-on-processing et hide-on-processing)

aaaah oki, j'avais pas vu, My bad !
Je valide ;)

@ptitloup ptitloup merged commit eb49ce2 into EsupPortail:develop Jan 30, 2024
5 checks passed
vsabatie pushed a commit to vsabatie/Pod that referenced this pull request Feb 5, 2024
* Check extesion allowed video filename

* Update add_video.html

Remove jquery part
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants