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 the weird links on the Footer #306

Merged
merged 7 commits into from
Aug 27, 2020
Merged

Fix the weird links on the Footer #306

merged 7 commits into from
Aug 27, 2020

Conversation

thienlnam
Copy link
Contributor

Log into the app on web, make sure the links iOS and Android look similar to the Desktop and Sign out links
Screen Shot 2020-08-27 at 8 35 44 AM

@thienlnam thienlnam self-assigned this Aug 27, 2020
@thienlnam thienlnam requested a review from tgolen August 27, 2020 15:36
@@ -259,6 +259,9 @@ const styles = {
sidebarFooterLink: {
color: '#C6C9CA',
fontSize: 11,
textDecorationLine: 'none',
fontFamily: 'GTAmericaExp-Regular',
lineHeight: '20px',
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check that this isn't going to make iOS puke?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I am testing this now

@shawnborton
Copy link
Contributor

I was also looking into this and I am a bit stumped. Typically we don't include any units on things like lineHeight, but RN should spit out a px for us to give us a desired lineHeight of 20px. However, on web, it seems like it stays at line-height: 20 which makes the style look super borked:
image

I know we have two different files for how we display the anchor component based on web or native:
image

Any ideas?

@shawnborton
Copy link
Contributor

It's strange that the fontSize does indeed get the px...

@tgolen
Copy link
Contributor

tgolen commented Aug 27, 2020

Are you sure you're setting lineHeight: 20 and not lineHeight: '20'? (notice the quotes around 20)

@shawnborton
Copy link
Contributor

Yup, I am sure I am setting it as a number and not a string.

@thienlnam
Copy link
Contributor Author

Yeah I ran into the same issue which is why I had to specifically declare '20px' instead of 20

@tgolen
Copy link
Contributor

tgolen commented Aug 27, 2020

OK, I think I know what's causing this. Since <a> (an HTML tag) is being rendered instead of a RN specific tag, that's why the lineHeight is being applied differently. I'm going to see if I can figure out some kind of better solution to this.

@shawnborton
Copy link
Contributor

Did you check iOS when doing that though? This is what I see:
image

@tgolen
Copy link
Contributor

tgolen commented Aug 27, 2020

OK, I think I know what we need to do. We can ONLY use <Anchor> for the links in comments... Everywhere else, we need to use this:

<Text style={{color: 'blue'}}
      onPress={() => Linking.openURL('http://google.com')}>
  Google
</Text>

This is kind of my bad... what I would suggest is renaming <Anchor> to be <AnchorForCommentsOnly>, then create <Anchor> and have it use the implementation above.

@thienlnam
Copy link
Contributor Author

Okay this is updated and ready for another look!

@shawnborton shawnborton merged commit ec184cd into master Aug 27, 2020
@shawnborton shawnborton deleted the jack-fixFooterLinks branch August 27, 2020 16:40
@shawnborton
Copy link
Contributor

🤦 My bad I meant to just approve these changes (I pulled and tested on both web and iOS) and not merge, I haven't had enough coffee yet. Apologies @tgolen !

@thienlnam
Copy link
Contributor Author

I am going to add this to my I need more coffee moments https://docs.google.com/document/d/10eIm0CQy8uqs9y4CtFbBEFgOLTACTnq9qoMNlzOE8aA/edit?usp=sharing

@tgolen
Copy link
Contributor

tgolen commented Aug 27, 2020

Haha, no worries, Shawn 👍 It's all good here. Thanks for the changes, Jack!

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