#1803 Fix the User Interface.
Closed 5 years ago by pingou. Opened 7 years ago by rahulbajaj.
rahulbajaj/pagure UI_Fixes  into  master

@@ -0,0 +1,11 @@ 

+ function gotoEdit(){

+   var change = $(".previewinmarkdown")[0];

+   if (change.innerHTML == "Edit"){

+     change.innerHTML = "Preview";

+     $(".Preview_label").hide();

+   }

+   else {

+     change.innerHTML = "Edit";

+     $(".Preview_label").show();

+   }

+ }

file modified
+13 -11
@@ -84,8 +84,9 @@ 

  

              <label for="comment"><strong>Add new comment</strong></label>

              <small class="text-muted pull-xs-right">

-               <span class="btn btn-sm btn-secondary inactive"

-                 aria-pressed="false" id="previewinmarkdown">Preview

+               <span class="Preview_label" style="display: none;">Previewing Comment</span>

+               <span class="btn btn-sm btn-secondary inactive previewinmarkdown"

+                 aria-pressed="false" onclick="gotoEdit()">Preview

                </span>

              </small>

              {% if repo.quick_replies %}
@@ -447,6 +448,7 @@ 

  <script type="text/javascript" src="{{ url_for('static', filename='selectize.min.js') }}"></script>

  <script type="text/javascript" src="{{ url_for('static', filename='atwho/jquery.caret.min.js') }}"></script>

  <script type="text/javascript" src="{{ url_for('static', filename='atwho/jquery.atwho.min.js') }}"></script>

+ <script type="text/javascript" src="{{ url_for('static', filename='pagure.js') }}"></script>

  

  <script type="text/javascript">

  
@@ -687,8 +689,8 @@ 

          {# The event-source server will automatically refresh the UI #}

          $('#comment').val('');

          $('#preview').html('');

-         $('#previewinmarkdown').addClass('inactive');

-         $('#previewinmarkdown').removeClass('active');

+         $('.previewinmarkdown').addClass('inactive');

+         $('.previewinmarkdown').removeClass('active');

          $('#preview').hide();

          $('#comment').show();

          $('#comments').find('.comment_body').show();
@@ -911,9 +913,9 @@ 

    });

  

    $( "#preview" ).hide();

-   $( "#previewinmarkdown" ).click(

+   $( ".previewinmarkdown" ).click(

      function(event, ui) {

-       if ($( "#previewinmarkdown" ).hasClass("inactive")){

+       if ($( ".previewinmarkdown" ).hasClass("inactive")){

          var _text = $( "#comment" ).val();

          var _url = "{{ url_for('markdown_preview',

                          repo=repo.name,
@@ -930,8 +932,8 @@ 

            success: function(res) {

              var preview = emojione.toImage(res)

              $( "#preview" ).html(preview);

-             $( "#previewinmarkdown" ).removeClass("inactive");

-             $( "#previewinmarkdown" ).addClass("active");

+             $( ".previewinmarkdown" ).removeClass("inactive");

+             $( ".previewinmarkdown" ).addClass("active");

              $( "#comment" ).hide();

              $( "#preview" ).show();

            },
@@ -940,9 +942,9 @@ 

            }

          });

          return false;

-       } else if ($( "#previewinmarkdown" ).hasClass("active")){

-         $( "#previewinmarkdown" ).addClass("inactive");

-         $( "#previewinmarkdown" ).removeClass("active");

+       } else if ($( ".previewinmarkdown" ).hasClass("active")){

+         $( ".previewinmarkdown" ).addClass("inactive");

+         $( ".previewinmarkdown" ).removeClass("active");

          $( "#comment" ).show();

          $( "#preview" ).hide();

        }

@@ -61,8 +61,10 @@ 

            <fieldset class="form-group">

              <label for="issue_content"><strong>Description</strong></label>

              <small class="text-muted pull-xs-right">

-               <span class="btn btn-sm btn-secondary inactive"

-                 aria-pressed="false" id="previewinmarkdown">Preview</span>

+               <span class="Preview_label" style="display: none;">Previewing Comment</span>

+               <span class="btn btn-sm btn-secondary inactive previewinmarkdown"

+                 aria-pressed="false" onclick="gotoEdit()">Preview

+               </span>

              </small>

              <textarea class="form-control" rows="8" id="issue_content" name="issue_content"

                  placeholder="Describe your issue">
@@ -137,6 +139,7 @@ 

  <script type="text/javascript"

      src="{{ url_for('static', filename='emoji/emojicomplete.js') }}">

  </script>

+ <script type="text/javascript" src="{{ url_for('static', filename='pagure.js') }}"></script>

  <script type="text/javascript">

  {% if authenticated and form %}

  $(document).ready(function() {
@@ -202,9 +205,9 @@ 

  

  $(function() {

    $( "#preview" ).hide();

-   $( "#previewinmarkdown" ).click(

+   $( ".previewinmarkdown" ).click(

      function(event, ui) {

-       if ($( "#previewinmarkdown" ).hasClass("inactive")){

+       if ($( ".previewinmarkdown" ).hasClass("inactive")){

          var _text = $( "#issue_content" ).val();

          var _url = "{{ url_for('markdown_preview',

                          repo=repo.name,
@@ -221,8 +224,8 @@ 

            success: function(res) {

              var preview = emojione.toImage(res);

              $( "#preview" ).html(preview);

-             $( "#previewinmarkdown" ).removeClass("inactive");

-             $( "#previewinmarkdown" ).addClass("active");

+             $( ".previewinmarkdown" ).removeClass("inactive");

+             $( ".previewinmarkdown" ).addClass("active");

              $( "#issue_content" ).hide();

              $( "#preview" ).show();

            },
@@ -231,9 +234,9 @@ 

            }

          });

          return false;

-       } else if ($( "#previewinmarkdown" ).hasClass("active")){

-         $( "#previewinmarkdown" ).addClass("inactive");

-         $( "#previewinmarkdown" ).removeClass("active");

+       } else if ($( ".previewinmarkdown" ).hasClass("active")){

+         $( ".previewinmarkdown" ).addClass("inactive");

+         $( ".previewinmarkdown" ).removeClass("active");

          $( "#issue_content" ).show();

          $( "#preview" ).hide();

        }

@@ -188,8 +188,10 @@ 

      <fieldset class="form-group">

        <label for="initial_comment"><strong>Initial Comment</strong></label>

        <small class="text-muted pull-xs-right">

-         <span class="btn btn-sm btn-secondary inactive"

-           aria-pressed="false" id="previewinmarkdown">Preview</span>

+         <span class="Preview_label" style="display: none;">Previewing Comment</span>

+         <span class="btn btn-sm btn-secondary inactive previewinmarkdown"

+           aria-pressed="false" onclick="gotoEdit()">Preview

+         </span>

        </small>

        {{ form.initial_comment(class_="form-control")|safe }}

        <div>
@@ -582,8 +584,9 @@ 

                <fieldset class="form-group">

                  <label for="comment"><strong>Add new comment</strong></label>

                  <small class="text-muted pull-xs-right">

-                   <span class="btn btn-sm btn-secondary inactive"

-                     aria-pressed="false" id="previewinmarkdown">Preview

+                   <span class="Preview_label" style="display: none;">Previewing Comment</span>

+                   <span class="btn btn-sm btn-secondary inactive previewinmarkdown"

+                     aria-pressed="false" onclick="gotoEdit()">Preview

                    </span>

                  </small>

                  {% if repo.quick_replies %}
@@ -845,6 +848,7 @@ 

  <script src="{{ url_for('static', filename='selectize.min.js') }}" type="text/javascript"> </script>

  <script type="text/javascript" src="{{ url_for('static', filename='atwho/jquery.caret.min.js') }}"></script>

  <script type="text/javascript" src="{{ url_for('static', filename='atwho/jquery.atwho.min.js') }}"></script>

+ <script type="text/javascript" src="{{ url_for('static', filename='pagure.js') }}"></script>

  

  <script type="text/javascript">

  function cancel_edit_btn() {
@@ -1069,9 +1073,9 @@ 

    );

  

    $( "#preview" ).hide();

-   $( "#previewinmarkdown" ).click(

+   $( ".previewinmarkdown" ).click(

      function(event, ui) {

-         if ($( "#previewinmarkdown" ).hasClass("inactive")){

+         if ($( ".previewinmarkdown" ).hasClass("inactive")){

            var _text = $( "#comment" ).val();

            var _url = "{{ url_for('markdown_preview',

                          repo=repo.name,
@@ -1088,8 +1092,8 @@ 

              success: function(res) {

                  var preview = emojione.toImage(res)

                  $( "#preview" ).html(preview);

-                 $( "#previewinmarkdown" ).removeClass("inactive");

-                 $( "#previewinmarkdown" ).addClass("active");

+                 $( ".previewinmarkdown" ).removeClass("inactive");

+                 $( ".previewinmarkdown" ).addClass("active");

                  $( "#comment" ).hide();

                  $( "#preview" ).show();

              },
@@ -1098,9 +1102,9 @@ 

              }

            });

            return false;

-         } else if ($( "#previewinmarkdown" ).hasClass("active")){

-             $( "#previewinmarkdown" ).addClass("inactive");

-             $( "#previewinmarkdown" ).removeClass("active");

+         } else if ($( ".previewinmarkdown" ).hasClass("active")){

+             $( ".previewinmarkdown" ).addClass("inactive");

+             $( ".previewinmarkdown" ).removeClass("active");

              $( "#comment" ).show();

              $( "#preview" ).hide();

          }
@@ -1192,8 +1196,8 @@ 

        if(data == 'ok') {

          $('#comment').val('');

          $('#preview').html('');

-         $('#previewinmarkdown').addClass('inactive');

-         $('#previewinmarkdown').removeClass('active');

+         $('.previewinmarkdown').addClass('inactive');

+         $('.previewinmarkdown').removeClass('active');

          $('#preview').hide();

          $('#comment').show();

          /* We have submitted the comment correctly */
@@ -1281,9 +1285,9 @@ 

  $(document).ready(function () {

    updateHighlight(true)

   {% if form %}

-   $( "#previewinmarkdown" ).click(

+   $( ".previewinmarkdown" ).click(

      function(event, ui) {

-       if ($( "#previewinmarkdown" ).hasClass("inactive")){

+       if ($( ".previewinmarkdown" ).hasClass("inactive")){

          var _text = $( "#initial_comment" ).val();

          var _url = "{{ url_for('markdown_preview') }}";

          $.ajax({
@@ -1297,8 +1301,8 @@ 

            success: function(res) {

              var preview = emojione.toImage(res);

              $( "#preview" ).html(preview);

-             $( "#previewinmarkdown" ).removeClass("inactive");

-             $( "#previewinmarkdown" ).addClass("active");

+             $( ".previewinmarkdown" ).removeClass("inactive");

+             $( ".previewinmarkdown" ).addClass("active");

              $( "#initial_comment" ).hide();

              $( "#preview" ).show();

            },
@@ -1307,9 +1311,9 @@ 

            }

          });

          return false;

-       } else if ($( "#previewinmarkdown" ).hasClass("active")){

-         $( "#previewinmarkdown" ).addClass("inactive");

-         $( "#previewinmarkdown" ).removeClass("active");

+       } else if ($( ".previewinmarkdown" ).hasClass("active")){

+         $( ".previewinmarkdown" ).addClass("inactive");

+         $( ".previewinmarkdown" ).removeClass("active");

          $( "#initial_comment" ).show();

          $( "#preview" ).hide();

        }

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 ?

2 new commits added

  • [RFE] Change name of Preview Button when showing Preview
  • Fix the name of pull_request comment button
7 years ago

@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?

rebased

7 years ago

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 :)

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.

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

@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

rebased

7 years ago

@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)

rebased

7 years ago

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 :)

Pull-Request has been closed by pingou

5 years ago