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

77 about page #156

Merged
merged 31 commits into from
Feb 23, 2024
Merged

77 about page #156

merged 31 commits into from
Feb 23, 2024

Conversation

ri-pandey
Copy link
Contributor

@ri-pandey ri-pandey commented Feb 12, 2024

Description

Made the About page's text editable via a markdown editor.

Related Issue(s)

Closes #77

If applicable, please reference the issue(s) that this PR addresses. If the PR does not address any specific issue, you can remove this section.

Changes Made

List the main changes made in this PR. Be as specific as possible.

  • Feature added

  • Code refactored

  • Other changes: [describe]

  • Made the About page editable via a markdown input.

  • Persisted markdown is rendered as HTML in the /about page, as well as in the Live Preview.

  • After retrieval, md.render() is used to convert persisted markdown to HTML, which strips out potentially-malicious elements from the markdown (like javascript:). The generated HTML is then sent to DOMPurify.sanitize() which is an HTML-sanitizer built for protection against XSS attacks. This sanitized HTML is then displayed to the user.

  • To continue allowing unauthenticated users into the /about page, the endpoint to fetch the latest About text is unauthenticated, and rate-limited for security.

Screenshots (if applicable)

Desktop View

Screenshot 2024-02-12 at 4 59 13 PM

Mobile View (horizontal scroll in effect)

Screenshot 2024-02-12 at 4 59 58 PM

Checklist

Before submitting this PR, please make sure that:

  • Your code passes linting and coding style checks.
  • Documentation has been updated to reflect the changes.
  • You have reviewed your own code and resolved any merge conflicts.
  • You have requested a review from at least one team member.
  • Any relevant issue(s) have been linked to this PR.

@ri-pandey ri-pandey self-assigned this Feb 12, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for a separate route file for the latest version? Could this be included in the main about.js route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The /about_latest route needs to be unauthenticated, since the About page is open regardless of whether or not a user is logged in. Hence the separate file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to above, latest could be /about/latest

Copy link
Contributor Author

@ri-pandey ri-pandey Feb 14, 2024

Choose a reason for hiding this comment

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

The endpoints under /about/ are exposed via api/routes/about.js, and are the ones requiring authentication. Having a /about/latest/ path would force us to move the unauthenticated endpoint to the about.js file (which is declared after the authentication middleware comes into effect), which would then force all of it's sub-paths (like /about/latest) to use authentication, which we are trying to avoid.

@deepakduggirala
Copy link
Contributor

deepakduggirala commented Feb 15, 2024

  1. /about can be registered before router.use(authenticate), this will remove auth from all routes starting with /about. Now, add authenticate to routes inside /about that require it. Refer to routes/auth.js which does the same.

  2. Please change PATCH to PUT for /about/:id

  3. UI: Instead of ? icon and popover text, directly show the text on top of the input ("Markdown supported")

  4. I like the split view with real-time updates, but in smaller screens can you show Write & Preview tabs ala GitHub?

image
  1. escape sanitizer is used on API which replaces <, >, &, ', " and / with HTML entities. Does this impact md to html render when the content is fetched back?

  2. As far as I understand, the sanitization is done just before rendering the HTML, this means that the DB stores potentially malicious markdown. It would be better to store the sanitized markdown or reject storing malicious markdown. (There has to be some checks on the API layer).

  3. Could you please add some comments to about.vue component on how it works? There are 2 versions of markdown and html states which I'm having trouble understanding.

@ri-pandey ri-pandey merged commit e45648d into dev Feb 23, 2024
ri-pandey added a commit that referenced this pull request Feb 23, 2024
* retrieve about text from api

* WIP

* escape input

* switched to markdown-it

* fixed some styles

* use toast to show errors

* size; reset after Cancel

* made height fixed; use break-words

* changed modal size

* changed Edit button

* allow users/operators to read

* some UI changes

* escape input; allow for creation or updating

* use popover to indicate syntax

* UI changes

* added unauthenticated route

* cleanup

* changed routes; UI changes

* WIP

* force equal heights; made break-words work

* use labels optionally

* WIP

* changed .gitignore; seed data

* WIP - HTML line breaks missing

* fixed several styles

* fixed spacing; fonts

* use constant height across different screen sizes

* labels as props

* edited seeding code
ri-pandey added a commit that referenced this pull request Mar 1, 2024
* 77 about page (#156)

* retrieve about text from api

* WIP

* escape input

* switched to markdown-it

* fixed some styles

* use toast to show errors

* size; reset after Cancel

* made height fixed; use break-words

* changed modal size

* changed Edit button

* allow users/operators to read

* some UI changes

* escape input; allow for creation or updating

* use popover to indicate syntax

* UI changes

* added unauthenticated route

* cleanup

* changed routes; UI changes

* WIP

* force equal heights; made break-words work

* use labels optionally

* WIP

* changed .gitignore; seed data

* WIP - HTML line breaks missing

* fixed several styles

* fixed spacing; fonts

* use constant height across different screen sizes

* labels as props

* edited seeding code

* cleanup (#171)

* made About editable to operators

* removed seed data

* added bootstrap data
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.

None yet

3 participants