This should be divided into two different PRs IMHO :smile:

Fixing the name was a small change so I though might as well fix it in this pr :)

I think I like the old one better

Ah, nevermind I see the idea

We're mixing two ways of accessing the dom, could we use one? (preferably the jquery way since that's also what we use elsewhere)

One small change the rest looks good :)

@pingou just out of curiosity can we not put the goToEdit to a centralised place where it can be accessed since this is repeating everywhere

There is a way to include templates in jinja, would be nice to benefit from this indeed

cf: http://stackoverflow.com/questions/22860085/include-html-file-in-jinja2-template#22860144

There is a way to include templates in jinja, would be nice to benefit from this indeed
cf: http://stackoverflow.com/questions/22860085/include-html-file-in-jinja2-template#22860144

@pingou from what i understand here is that, we can make a new template in which we can have the goToEdit function and then i can {% include %} it in the the templates as required to avoid re-writing of code.
Am i correct ?

IMHO we are not going to use the goToEdit function anywhere except for the preview button, therefore i think its okay as is, what is your opinion ?

correct, i can do that :)

@pingou, @farhaan I have used jquery to avoid using different dom :) Does gotoEdit() need to be in a different template in your opinion ?

Do you really need the .get(0) ?

If the exact same piece of code is used across multiple places then it should definitively be moved to either its own template, or a macro that can be called where needed.

Actually, since these changes are all JS, it would make sense to add them to a pagure.js file that would be loaded by these pages, potentially with a function to set things up.

Any news on this one?

@pingou @farhaan Looking good ?

Do we want to make this identifier a little more specific? This seems quite generic

This one doesn't have the onclick instruction, is that expected?

This is the second time this identifier is used in that page, I believe it's not allowed

No, that is not expected. I shall add the onclick feature to it. thanks :)

will do that :)

I am not quite sure what what should be done to avoid this. But why we can not use two identifiers doing the same job twice ? We are also using "previewinmarkdown" and "preview" identifiers twice.

@pingou , looking good ?

@pingou as discussed on IRC I have managed to avoid two identifiers on the same page. Is the Patch appropriate ?

The code I'm seeing is still using twice the same identifier in the same page:
- https://pagure.io/pagure/pull-request/1803#_4,8
and
- https://pagure.io/pagure/pull-request/1803#_4,20

@pingou hey, i have made changes to avoid the duplication of identifiers but is this the correct way to do it ?

This will need to be rebased before it can be merged (it conflicts atm)

@pingou done :)

The two tickets fixed in this PR have been fixed in another PR, so I am going to close this PR now.

Sorry I couldn't merge it in a timely fashion :(

Please keep contributing to pagure :)