-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#467] Add Title Component #2102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi @sopa301, thanks for working on this! I have not tested your code yet, but after skimming through the changes, I have some thoughts.
|
Currently, the content is obtained by looking for a Regarding prof @damithc 's query of how it looks: Html report (the whole screen): The rest of the report can be accessed by scrolling down and the title can be removed from view by scrolling down. |
Sounds promising @sopa301 Is there any live preview for me to see how it work? |
Unfortunately I don't have a way to provide a live preview now. Also there's a bug in the tests, so the github preview doesn't work. I can show it to you in person next monday if I don't solve the bug by then. If someone knows of another way to host a preview do let me know! If the title.md is empty, the report will look the same as before. I forgot to handle the case where the file is missing, but I think I'll rewrite the code to account for this scenario. |
Generate the report locally and upload to a temp github.io site? |
I managed to host the html file here, although I messed up somewhere and the data didn't get uploaded as well. I'll update the link if I got it fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I would recommend getting approval for the UI from Prof as well, either by deploying an example or demoing it during the lecture slot.
Co-authored-by: Charisma Kausar <68203159+ckcherry23@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this @sopa301!
Just wanted to highlight some things about the usage of v-html
: There are some pretty important security concerns we need to be mindful of when using this, since rendering HTML that's partly controlled by users has the potential to lead to XSS vulnerabilities.
Reference: Vue's docs and React's equivalent (React even deliberately named their property dangerouslySetInnerHTML
)
Did some reading and it seems like it's OK in this case, as the library we're using (Markdown-it) claims to output safe HTML as long as the raw HTML option is disabled (which it is by default). Just wanted to highlight this point to make sure we are mindful of it if we want to introduce any more advanced features like custom styling in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sopa301 Would it be possible to add tests for the title component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Will wait for approval re: the UI from prof as per @ckcherry23 above before merging
I can implement some tests to check for correct handling of exceptions. I think this would be a good time to try to integrate Jest since this component needs to handle negative cases and needs unit testing. On the other hand, I don't think testing the markdown itself should be necessary, right? |
Yes, since rendering the Markdown is handled by an external library, we don't have to test it. Since we have to do some setup for the unit tests, we could do that in a different PR. |
I have discussed this with Prof @damithc and he suggested allowing raw HTML so that we can do styling with the |
The following links are for previewing this pull request:
|
@@ -31,7 +31,8 @@ The section below provides explanations for each of the flags. | |||
<div id="section-assets"> | |||
|
|||
**`--assets ASSETS_DIRECTORY`**: Specifies where to place assets for report generation. | |||
* Parameter: `ASSETS_DIRECTORY` The directory containing the assets files. A `favicon.ico` file can be placed here to customize the favicon of the dashboard. | |||
* Parameter: `ASSETS_DIRECTORY` The directory containing the assets files. A `favicon.ico` file can be placed here to customize the favicon of the dashboard, | |||
while a `title.md` file can be placed to customize the header of the report using [Markdown syntax](https://www.markdownguide.org/basic-syntax/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sopa301 Noticed while passing: There should not be a line break here (in markdown files, we should not wrap lines to a specific length).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sopa301 Further comments:
- There should be more details of this feature. For example, code for an example title component.
- Doesn't feel like this is the right place to document this feature. Perhaps add as a separate section in https://reposense.org/ug/customizingReports.html ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, perhaps add a title component to our preview dashboard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review prof, I'll work on it!
Fixes #467
Proposed commit message
Other information
The input file 'title.md' is inserted with the --assets flag when the jar file is used.