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: tooltip content on the registration form #89

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

ymaheshwari1
Copy link
Contributor

  1. Improved the tooltip content for the screen name in the registration form

@ymaheshwari1
Copy link
Contributor Author

ymaheshwari1 commented May 22, 2021

Hi,

As I am looking around the roller, found that when using struts2 tags the tooltip attribute does not support HTML, so should we remove HTML from the tooltip text?

Text passed as tooltip content: https://github.com/apache/roller/blob/master/app/src/main/resources/ApplicationResources.properties#L1451**

UI:
image

@mbien
Copy link
Member

mbien commented May 22, 2021

the bootstrap ui does still support html in tooltips, its just disabled by default.
see https://getbootstrap.com/docs/3.4/javascript/#tooltips-options

I am not sure what would be the best way to enable it. Doesn't look like struts lets you set a data attribute.

@adityasharma7
Copy link
Member

Hi @ymaheshwari1 @mbien,

For the current case, I think we can consider removing the HTML as there are security concerns enabling the HTML mode.

WDYT?

@ymaheshwari1
Copy link
Contributor Author

I think, yes we can do that, as the HTML is used only in some tooltips.

@mbien
Copy link
Member

mbien commented May 28, 2021

Hi @ymaheshwari1 @mbien,

For the current case, I think we can consider removing the HTML as there are security concerns enabling the HTML mode.

WDYT?

yeah sure, there are not that many anyway. I was checking if we could maybe enable some selectively for strings which are known to be static. But yeah lets just remove them.

are those properties also tooltips or are they used somwhere else?

userRegister.tip.openid.only=This site uses only OpenID for logins, so please \
specify your OpenID identifier below. For more information about OpenID see \
<a href=\"http://openid.net\">http://openid.net</a>.

since they contain links.

@ymaheshwari1
Copy link
Contributor Author

ymaheshwari1 commented May 28, 2021

Hi @ymaheshwari1 @mbien,
For the current case, I think we can consider removing the HTML as there are security concerns enabling the HTML mode.
WDYT?

yeah sure, there are not that many anyway. I was checking if we could maybe enable some selectively for strings which are known to be static. But yeah lets just remove them.

are those properties also tooltips or are they used somwhere else?

userRegister.tip.openid.only=This site uses only OpenID for logins, so please \
specify your OpenID identifier below. For more information about OpenID see \
<a href=\"http://openid.net\">http://openid.net</a>.

since they contain links.

Hi, These are displayed when the authMethod != 'LDAP'. Also, they are not tooltips.

https://github.com/apache/roller/blob/master/app/src/main/webapp/WEB-INF/jsps/core/Register.jsp#L73

But, as they are used in s:text, it only considers string in it.
https://struts.apache.org/tag-developers/text-tag.html

@ymaheshwari1
Copy link
Contributor Author

Will handle the issue of HTML in tooltips in another MR, hence making it ready for review.

@ymaheshwari1 ymaheshwari1 marked this pull request as ready for review June 2, 2021 15:09
@mbien
Copy link
Member

mbien commented Jun 2, 2021

thanks for the fix looks good.

@mbien mbien merged commit b9b5ff3 into apache:master Jun 2, 2021
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.

3 participants