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

Link to latest in scenario name #836

Merged

Conversation

okurz
Copy link
Member

@okurz okurz commented Aug 20, 2016

Screenshot:
openqa_link_to_latest_in_previous

Based on 'latest' query route each job can now link to the
newest incarnation within in the same scenario.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 71.03% when pulling 13199c2 on okurz:feature/link_to_latest_in_scenario_name into bc7a31d on os-autoinst:master.

@okurz okurz force-pushed the feature/link_to_latest_in_scenario_name branch from 13199c2 to 3a2842b Compare August 24, 2016 20:26
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 71.054% when pulling 3a2842b on okurz:feature/link_to_latest_in_scenario_name into 3e440e8 on os-autoinst:master.

@@ -1,5 +1,5 @@
<div id="scenario">
<div class="h5">Results for <i><%= $scenario %></i></div>
<div class="h5">Results for <i><%= link_to($scenario => url_for ('latest')->query($job->scenario_hash)) %></i></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried it and it's confusing for me. I would prefer something like:

<div class="h5">Results for <i><%= $scenario %></i> (<%= link_to('latest results for this scenario' => url_for('latest')->query($job->scenario_hash)) %>)</div>

Copy link
Member Author

Choose a reason for hiding this comment

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

On Friday 26 August 2016 07:26:07 Ondřej Holeček wrote:

@@ -1,5 +1,5 @@

-
Results for <%= $scenario %>
-
Results for <%= link_to($scenario => url_for ('latest')->query($job->scenario_hash)) %>
Tried it and it's confusing for me. I would prefer something like:
<div class="h5">Results for <i><%= $scenario %></i> (<%= link_to('latest
results for this scenario' =>
url_for('latest')->query($job->scenario_hash)) %>)</div> ```

sure, I wouldn't mind

Copy link
Contributor

Choose a reason for hiding this comment

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

latest results implies it's multiple results, but it's just one -> 'latest job' then

@okurz
Copy link
Member Author

okurz commented Aug 31, 2016

updated:

  • made link more explicit as suggested by @aaannz
  • added screenshot to description

@coolo your opinion?

@coolo
Copy link
Contributor

coolo commented Sep 1, 2016

other than the wording I'm fine with the change

@okurz okurz force-pushed the feature/link_to_latest_in_scenario_name branch from cceb92f to e54f550 Compare September 1, 2016 14:32
@okurz
Copy link
Member Author

okurz commented Sep 1, 2016

@aaannz the trim_whitespace failed me in https://travis-ci.org/os-autoinst/openQA/builds/156827974

@okurz okurz force-pushed the feature/link_to_latest_in_scenario_name branch from e54f550 to 70c5c83 Compare September 1, 2016 15:49
@okurz
Copy link
Member Author

okurz commented Sep 1, 2016

@coolo, @aaannz updated wording, just using regex match for the test to not replicate the full text content and confuse trim_whitespace. Please merge and deploy, should make anyone happy asking in bugs: "Does this still happen?" ;-)

@aaannz
Copy link
Contributor

aaannz commented Sep 1, 2016

I don't know, trim_whitespace replace multiple spaces by one and removes spaces at the beginning and end of the string. From my limited perspective it seems to me you wanted to remove one space near () inside of the string.

@aaannz
Copy link
Contributor

aaannz commented Sep 1, 2016

btw. LGTM :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 70.994% when pulling 70c5c83 on okurz:feature/link_to_latest_in_scenario_name into 8609e09 on os-autoinst:master.

@okurz
Copy link
Member Author

okurz commented Sep 1, 2016 via email

@aaannz aaannz merged commit 132350b into os-autoinst:master Sep 2, 2016
@okurz okurz deleted the feature/link_to_latest_in_scenario_name branch September 2, 2016 07:49
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.

4 participants