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

Improved interpreting text as markdown #6095

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Apr 15, 2024

Closes #5508

Why is this the best possible solution? Were any other approaches considered?

We are trying to keep the way we interpret _ markdown close to what GitHub does as this seems to be standard. I was thinking about adding a library for doing that instead of writing all those rules on our own but it looks like there is no official one that would be safe to use.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It introduces improvements around how we interpret _ as markdown. #5685 was closed some time ago by #5685 but recently we have realized it didn't cover all cases see: #5508 (comment). You can review tests I have updated if you need help understanding https://github.com/getodk/collect/compare/master...grzesiek2010:collect:COLLECT-5685_q?expand=1#diff-016f69200d89adf3c39980de01acc160e045b3f93c79e7745dac1c4c6f1e33f4R38.
Generally, it's only related to using underscores (_) so we can focus on testing only this case.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 requested a review from seadowg April 15, 2024 07:41
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

Is this meant to reference a different issue? #5685 is a PR.

@grzesiek2010
Copy link
Member Author

Is this meant to reference a different issue? #5685 is a PR.

Yes it should be #5508

@grzesiek2010 grzesiek2010 requested a review from seadowg April 16, 2024 11:47
alxndrsn pushed a commit to alxndrsn/enketo-monorepo that referenced this pull request Apr 17, 2024
@seadowg seadowg merged commit 73b812b into getodk:master Apr 23, 2024
6 checks passed
@dbemke
Copy link

dbemke commented May 10, 2024

@grzesiek2010 Could you help us a bit which parts exactly to test? The link to tests shows
githubLink
Is it the same as in "Files changed" tab?
Do changes in interpreting text as markdown influence only answers or also heading? Should we test answers only in the form or also e.g. in selects in attached csv, xml, geojson?

@dbemke
Copy link

dbemke commented May 13, 2024

Another thing I'm not sure how should work - if I see underscore in answers in a question in Collect when I send it to Central should the underscore be interpreted in the same way?

@dbemke
Copy link

dbemke commented May 20, 2024

@grzesiek Should the underscore be interpreted in the same way when it's in the name field and the label field in a form?
Here's and example of selects in a form:
underscorCollCentr
This what Collect sees ( answer C is different):
collectUnderscores
This is what Central sees:
centralSubmissions

@grzesiek2010
Copy link
Member Author

Should the underscore be interpreted in the same way when it's in the name field and the label field in a form?

no, the value should remain unchanged as it's what we use to identify choices. Central displays the values and it's ok.

Another thing I'm not sure how should work - if I see underscore in answers in a question in Collect when I send it to Central should the underscore be interpreted in the same way?

it's up to Central if styling is supported then yes. But it's not important from Collect's point of view.

@grzesiek2010 Could you help us a bit which parts exactly to test? The link to tests shows Is it the same as in "Files changed" tab?

The tests are here https://github.com/getodk/collect/pull/6095/files#diff-016f69200d89adf3c39980de01acc160e045b3f93c79e7745dac1c4c6f1e33f4R38
We have discussed this last week but if you need to know something more please let me know.

Do changes in interpreting text as markdown influence only answers or also heading?

As we discussed last week everywhere we handle markdown the text should be styled:

  • group names
  • question labels
  • hints
  • guidance text
  • choices in select questions
  • answers in the hierarchy view

Should we test answers only in the form or also e.g. in selects in attached csv, xml, geojson?

I don't think it's important to test different sources of text that should be styled.

@dbemke
Copy link

dbemke commented May 20, 2024

Should it be also in dialogs? Example: SEA 774 BOB - 774 should be in italics in repeat group dialog.
repeatUnderscore

@grzesiek2010
Copy link
Member Author

Should it be also in dialogs? Example: SEA 774 BOB - 774 should be in italics in repeat group dialog.

We have never handled markdown in those group dialogs so the answer for now is no. I think you can file an issue for that. Apparently, it's not very important as no one has complained about it but we should be consistent and if it is possible, style the group names there too.

@dbemke
Copy link

dbemke commented May 20, 2024

There are 2 differences in examples from your tests.
testVsCollect
The first label (in the screenshot) in Collect shows B in italics while github shows this A\*B\*C.
The second label in Collect shows AB\ in bold while in tests it is supposed to be in italics (github shows the same result as in Collect)
The form with examples
https://test.getodk.cloud/#/projects/436/forms/SelectFormFileMarkdown/draft

@lognaturel
Copy link
Member

I think you can file an issue for that.

I don't think we need to do that. The repeat prompt dialog uses label text which is determined by the form designer. It's intended to be something human-readable.

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented May 21, 2024

The first label (in the screenshot) in Collect shows B in italics while github shows this A*B*C.
The second label in Collect shows AB\ in bold while in tests it is supposed to be in italics (github shows the same result as in Collect)

This pr is only about styling with underscores _ not with asterisks *. There might be some inconsistencies with asterisks and we are aware of that. You can file a separate issue for that.

To be specific the first example A\\*B\\*C in Collect should be displayed as A*B*C in our project the doubled backslashes mean the same as single ones here in Github (they are used to escape the asterisk). They just need to be doubled in Android Studio to work properly. So A\\*B\\*C is the same as A\*B\*C here in GitHub and in both cases, the result should be A*B*C.

The second example \\**AB\\** in Android Studio is the same as \**AB\** in Github and the result should be *AB*.

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented May 21, 2024

I don't think we need to do that. The repeat prompt dialog uses label text which is determined by the form designer. It's intended to be something human-readable.

But we do support styling group names displayed in paths and it's the same group label used in both places so why not to support styling in both cases?

@WKobus
Copy link

WKobus commented May 23, 2024

Tested with Success!

Verified on device with Android 14

Verified cases:

  • Styling in group names, question labels, hints, guidance text, choices in select questions, answers in the hierarchy view
  • Styling in select choices from external file
  • Different styling options
  • Regression check on styling

@dbemke
Copy link

dbemke commented May 23, 2024

Tested with Success!

Verified on device with Android 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only treat _ as markdown if separated by white space
5 participants