Conversation
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
Overall this LGTM! I've just left a couple of comments / questions.
| {% set flow = summary.flow_results[block_name] %} | ||
| <tr> | ||
| <td class="p-0"> | ||
| <a href="{{ base_url }}{{ block_name }}.html" |
There was a problem hiding this comment.
If it isn't too hard, can you either:
- Omit the button / hyperlink if no base URL is given, or
- Get the base URL from the
index.jsonpath parent if it is not provided.
Depending on whatever the intended behaviour is, because at the moment if I run this locally without specifying --base-url I just get broken links. I suspect the intended behaviour is (1), so that if we generate a dashboard without giving it a link to find block reports (e.g. on a hosted site), it just doesn't link anywhere.
I think it would be better to have base_url be str | None (default None), and then only link in the template if base_url is not none. That way you can still use --base_url "" if you want links local to the dashboard output directory and still have the links be defined.
There was a problem hiding this comment.
That should be working now.
machshev
force-pushed
the
dashboard
branch
2 times, most recently
from
0ebe1ea to
a06e80f
Compare
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
I appreciate that all the changes may not yet have been made, but I've left a couple more nits for the latest pushes (sorry!)
machshev
force-pushed
the
dashboard
branch
2 times, most recently
from
29b3636 to
d27f2f0
Compare
This function can be used for more than just the HTML report CSS and JS rendering. So pull it out and add unit tests. Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
AlexJones0
left a comment
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks @machshev, this LGTM.
2 participants
Add support for generating a dashboard.