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

Fix NcRichText style #3932

Merged
merged 2 commits into from Mar 28, 2023
Merged

Fix NcRichText style #3932

merged 2 commits into from Mar 28, 2023

Conversation

julien-nc
Copy link
Contributor

@julien-nc julien-nc commented Mar 27, 2023

@juliushaertl We missed something when moving RichText in here as NcRichText.
The richtext.scss file is not used anymore. It had to be imported separately when RichText was used.
We could now simply import it in NcRichText.

@raimund-schluessler No problem with importing it, right?

Another problem raised in nextcloud/spreed#9163 is that links have the rich-text--component class and they apparently used to have the rich-text--external-link (with useMarkdown set to false). This leads to them not being underlined anymore.
@juliushaertl Apparently this appeared after @nextcloud/vue-richtext v2.0.4. Does it ring a bell? Any intentional change in the link class?

Fix nextcloud/spreed#9163

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc added bug Something isn't working feature: richtext Related to the richtext component labels Mar 27, 2023
@julien-nc julien-nc added this to the 7.8.5 milestone Mar 27, 2023
@raimund-schluessler
Copy link
Contributor

@raimund-schluessler No problem with importing it, right?

I think that should be fine.

@juliushaertl
Copy link
Contributor

Another problem raised in nextcloud/spreed#9163 is that links have the rich-text--component class and they apparently used to have the rich-text--external-link (with useMarkdown set to false). This leads to them not being underlined anymore.

I think the class should still be there pulled in from https://github.com/nextcloud/nextcloud-vue/blob/fix/noid/ncrichtext-style/src/components/NcRichText/autolink.js#L69
https://github.com/nextcloud/nextcloud-vue/blob/fix/noid/ncrichtext-style/src/components/NcRichText/autolink.js#L20

Apparently this appeared after @nextcloud/vue-richtext v2.0.4. Does it ring a bell? Any intentional change in the link class?

Seems quite odd, nothing I'd be aware of, but might be that another app had the style imported and now that isn't the case anymore?

@juliushaertl
Copy link
Contributor

Just importing would be fine I think as well

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
Copy link
Contributor Author

I think I found the issue about the class. NcLink was functional and it is not anymore. It seems when rendering the component there:
https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/NcRichText/placeholder.js#L46-L50
the class was not overridden when NcLink was functional. I don't know if it's an expected difference between rendering functional and non-functional components.
Anyway, as the class is systematically overridden, we always loose the initial one set in the NcLink definition.

I implemented a basic fix which does not override the class of NcLink but still set the rich-text--component class for all other components.

@julien-nc julien-nc marked this pull request as ready for review March 28, 2023 09:31
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Fixes missing underlining for links to github etc.
But links to 127.0.0.1 are still not clickable :(

@nickvergessen nickvergessen added 3. to review Waiting for reviews regression Regression of a previous working feature labels Mar 28, 2023
@julien-nc
Copy link
Contributor Author

@juliushaertl Just wanna make sure you agree with this change 😁. Are there potentially other specific cases (other components) for which the class should not be overridden?

@nickvergessen nickvergessen merged commit 9f47785 into master Mar 28, 2023
15 checks passed
@nickvergessen nickvergessen deleted the fix/noid/ncrichtext-style branch March 28, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: richtext Related to the richtext component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links are not visible anymore
5 participants