-
Notifications
You must be signed in to change notification settings - Fork 3k
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
override default anchor style with parent style #5060
override default anchor style with parent style #5060
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
From screens, looks like text is not vertical aligned and relatively large for link. |
Hi @parasharrajat, If you check the screenshot in the original issue description, you can see similar styling there too. So I don't think that this PR brings along those problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM
Heads up @pROFESOR11 there are some merge conflicts on this PR. |
Hi @roryabraham and @marcaaron, |
@@ -72,6 +72,7 @@ function AnchorRenderer({tnode, key, style}) { | |||
// An auth token is needed to download Expensify chat attachments | |||
const isAttachment = Boolean(htmlAttribs['data-expensify-source']); | |||
const fileName = lodashGet(tnode, 'domNode.children[0].data', ''); | |||
const parentStyle = lodashGet(tnode, 'parent.styles.nativeTextRet', {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks OK to me, but seems potentially brittle because it's relying on some internal detail of the html library we are using.
e.g. I'm not really sure what parent.styles.nativeTextRet
is or whether we can depend on it to always exist? I also don't have an alternative suggestion so I'm fine with this for now - but would have preferred more comments in the code to add context about why we are doing this.
@pROFESOR11, Great job getting your first Expensify/App pull request over the finish line! 🎉 I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:
So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.0.98-2 🚀
|
Details
if there are specific styles being applied to the parent element that conflict with the default styles of the link, this PR will give preference to the parent style.
Fixed Issues
$ #4897
Tests
QA Steps
www.google.comTested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android