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

Adding Markdown support to Legend text #1279

Merged

Conversation

gislawill
Copy link
Collaborator

@gislawill gislawill commented Jun 18, 2024

Description

This addresses feature request #1233 by adding markdown support for the Legend text in the LegendItem component.

I chose to use the react-markdown library due to it's widespread usage and simple API. There are many more capabilities and customizations we can choose to adopt in the future.

This PR adds support for italics, bold, and links. There are many more elements we could add support for (i.e. an underline without link, strikethrough, headers, tables, lists, etc) — we'll just need to ensure the styling works for our needs.

Note: we're not using the latest version of react-markdown (v9) because it requires an updated version of react (v18 compared to our v16). I don't think we're missing out on much in v9 tbh.

Note 2: by default, links generated by markdown open in the current tab. We can override to open in a new tab if preferred (I do think that's what users would expect so I plan to add this). Updated ✅

How to test the feature:

@wadhwamatic are there any immediate use-cases for this markdown? If so, I can update the json now so we can use real examples for QA.

Checklist - did you ...

Test your changes with

  • REACT_APP_COUNTRY=rbd yarn start
  • REACT_APP_COUNTRY=cambodia yarn start
  • REACT_APP_COUNTRY=mozambique yarn start
  • Add / update necessary tests?
  • Add / update outdated documentation?

Screenshot/video of feature:

Example usage:
Screenshot 2024-06-18 at 9 36 12 AM

Generated html:
Screenshot 2024-06-18 at 9 41 18 AM

@gislawill gislawill linked an issue Jun 18, 2024 that may be closed by this pull request
// https://github.com/remarkjs/react-markdown/issues/635
jest.mock('react-markdown', () => (props: { children: React.ReactNode }) => {
return <>{props.children}</>;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is unfortunately, I would have rather rendered this properly in our jest tests. However, the alternative approach seemed more frustrating to maintain than the value it would provide.

See: remarkjs/react-markdown#635

Example alternative:
Screenshot 2024-06-18 at 10 00 54 AM

Copy link

github-actions bot commented Jun 18, 2024

Build succeeded and deployed at https://prism-1279.surge.sh
(hash f32a6b6 deployed at 2024-06-26T05:02:29)

@ericboucher
Copy link
Collaborator

@gislawill can you add a temp example in one oft the mozambique legends for this PR?

@gislawill
Copy link
Collaborator Author

@gislawill can you add a temp example in one oft the mozambique legends for this PR?

Can do! Just added an example under INAM aggregate rainfall
Screenshot 2024-06-21 at 8 25 31 AM

Copy link
Collaborator

@ericboucher ericboucher left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

</Typography>
),
}}
allowedElements={['p', 'h5', 'strong', 'em', 'a']}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to limit elements here? If so, it might be useful to reference allowed elements somewhere else. In the readme maybe or next to the the legendText type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Many of the theoretically allowable elements would not have acceptable formatting here (thinking of examples like tables, lists, code, etc). Rather than try to build reasonably acceptable formatting for all of these elements, I decided to limit to the elements I believe this feature request is intended for.

Good idea, I'll add a note to the readme what exactly is supported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated ✅

@gislawill
Copy link
Collaborator Author

gislawill commented Jun 26, 2024

@wadhwamatic, added the following links. The CHIRPS update was applied in every legend text that had the hyperlink (including translation files)
Screenshot 2024-06-25 at 9 39 06 PM
Screenshot 2024-06-25 at 9 39 43 PM
Screenshot 2024-06-25 at 9 48 39 PM

@gislawill gislawill marked this pull request as ready for review June 26, 2024 04:54
Copy link
Member

@wadhwamatic wadhwamatic left a comment

Choose a reason for hiding this comment

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

Tested across multiple countries / scenarios. All looks good. Nice work @gislawill

@wadhwamatic wadhwamatic merged commit 647c990 into master Jun 26, 2024
6 checks passed
@wadhwamatic wadhwamatic deleted the 1233-feature-request-support-markdown-in-layer-legend-text branch June 26, 2024 17:51
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.

[Feature Request]: Support markdown in layer legend text
3 participants