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

Add status bar to Test Run page #1436

Conversation

asankov
Copy link
Member

@asankov asankov commented Mar 17, 2020

No description provided.

@asankov asankov self-assigned this Mar 17, 2020
@coveralls
Copy link

coveralls commented Mar 17, 2020

Coverage Status

Coverage remained the same at 75.496% when pulling ab774d1 on asankov:test-run-patternfly-status-bar into 1719d29 on kiwitcms:refactor_testrun_ui.

tcms/testruns/static/testruns/js/get.js Outdated Show resolved Hide resolved
tcms/testruns/static/testruns/js/get.js Outdated Show resolved Hide resolved
tcms/testruns/static/testruns/js/get.js Outdated Show resolved Hide resolved
tcms/testruns/static/testruns/js/get.js Show resolved Hide resolved
tcms/testruns/static/testruns/js/get.js Outdated Show resolved Hide resolved
tcms/testruns/templates/testruns/get.html Outdated Show resolved Hide resolved
<li class="list-group-item" style="padding: 0;">
<label>${status}</label> - ${element}
</li>
`)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like rendering HTML inside JavaScript. The HTML template can render all status elements with their labels and count 0. It can also add data attribute or ID so these elements are easier to select via JS and then renderCountPerStatusList will only update the counts foe every list item, possibly only for 1 li based on the status ID (can be used later when updating TEs).

Copy link
Member Author

Choose a reason for hiding this comment

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

'HTML in JS' is used in ReactJS and is becoming quite standard thing, so it is not something weird. Also, it is the better approach for something that is going to be changed dynamically (e.g. when a button is clicked, or an event is triggered). Because you just delete the whole thing and render it again. Also, the new JS template strings makes in easy and neat.

IMO this is way better than selecting multiple elements and editing them one by one, because:

  1. DOM operations are slow. Deleting part of the DOM and inserting something new in its place is faster than selecting 10 elements by ID and changing them value one by one, because these are 10 DOM manipulations (vs 1 in the other approach)(and this is the way React works - it has a virtual DOM in memory, and even if you edit 10 elements, under the hood it groups them and in the end applies the changes as 1 DOM manipulation).
  2. It's one thing to select elements by IDs like status-button or status-bar and another to have to build IDs once in the HTML templates and another time in the JS. It's just ugly and will quickly get out of hand.

Copy link
Member

Choose a reason for hiding this comment

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

HTML in JS' is used in ReactJS and is becoming quite standard thing

this is not a reason to use something.

  1. DOM operations are slow.

with all the status li's rendered via the templating layer you are not deleting DOM elements, just updating their innerHTML attribute to the correct number value. This is something that is done anyway upon update of test executions to refresh the UI. Much better to have a minimalist 1 line update (or a small function if you like) and use it in several places instead of rendering

  • tags whenever this card needs to be updated.

    1. It's one thing to select elements by IDs like status-button or status-bar and ...
    $(`#status-${statusCount[status].id`).html(count)
    

    That's hardly getting out of hand and definitely less ugly than building the HTML contents in 2 different pieces of code.

    Also with the current implementation the labels with value 0 are not clickable which introduces unnecessary login on the FE. All of them should be exactly the same minus the text content.

  • Copy link
    Member Author

    Choose a reason for hiding this comment

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

    this is not a reason to use something.

    sure, it isn't. but 'I don't like something' isn't one as well as that is subjective. My point was that this is not something weird, nor something custom that only we are doing.

    definitely less ugly than building the HTML

    that is subjective. my opinion differs.

    Much better to have a minimalist 1 line update (or a small function if you like) and use it in several places instead of rendering

    again, this is subjection. I don't see anything bad with rendering.

    Also with the current implementation the labels with value 0 are not clickable which introduces unnecessary login on the FE

    I don't see how this concern this discussion. if the behavior should be different, it can be achieved both ways.

    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    The HTML template can render all status elements with their labels and count 0.

    No, it can't, because since 6189ad4
    the additional data that was sent by the view is gone.

    Also, this is not new code, it's just copied from 1dd0dac#diff-f1f966298171ba7b8ab6c743bd88e634R738-R745

    So, changing the approach would make the things more ugly in both the template, js, and the already simplified view.

    @asankov asankov force-pushed the test-run-patternfly-status-bar branch from ff9a4e6 to 1a21c4c Compare March 19, 2020 09:48
    @asankov asankov force-pushed the test-run-patternfly-status-bar branch from 1a21c4c to ab774d1 Compare March 28, 2020 17:47
    @codecov
    Copy link

    codecov bot commented Mar 28, 2020

    Codecov Report

    Merging #1436 into refactor_testrun_ui will not change coverage by %.
    The diff coverage is n/a.

    Impacted file tree graph

    @@                 Coverage Diff                  @@
    ##           refactor_testrun_ui    #1436   +/-   ##
    ====================================================
      Coverage                76.11%   76.11%           
    ====================================================
      Files                      122      122           
      Lines                     4384     4384           
      Branches                   510      510           
    ====================================================
      Hits                      3337     3337           
      Misses                     875      875           
      Partials                   172      172           

    Continue to review full report at Codecov.

    Legend - Click here to learn more
    Δ = absolute <relative> (impact), ø = not affected, ? = missing data
    Powered by Codecov. Last update c24008e...1618480. Read the comment docs.

    @asankov
    Copy link
    Member Author

    asankov commented Apr 7, 2020

    @atodorov any more comments on this?

    @atodorov
    Copy link
    Member

    atodorov commented Apr 9, 2020

    #1436 (comment)

    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