Skip to content
This repository has been archived by the owner on Apr 3, 2021. It is now read-only.

Fix: show link on light theme #49

Merged
merged 2 commits into from
Oct 15, 2019

Conversation

Shubhamchinda
Copy link
Contributor

@helfi92
Issue

The build issue got fixed after restarting vs-code 😅

Let me know if this is correct!
Thanks as always!

src/components/Markdown/index.jsx Outdated Show resolved Hide resolved
src/components/Markdown/index.jsx Outdated Show resolved Hide resolved
@Shubhamchinda
Copy link
Contributor Author

@djmitche
Made the changes! :)

@djmitche djmitche requested review from djmitche and removed request for djmitche October 2, 2019 19:46
Copy link

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Looks good to me! @helfi92 will be back next week, and will offer his review at that time.

@helfi92
Copy link
Contributor

helfi92 commented Oct 9, 2019

Planning to review this very soon :)

@Shubhamchinda
Copy link
Contributor Author

Planning to review this very soon :)

Okay 👍 Thanks!

color:
theme.palette.common[
theme.palette.type === 'light' ? 'black' : 'white'
],
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about removing this style all together and do the change in ErrorPanel/index.jsx:

diff --git a/src/components/ErrorPanel/index.jsx b/src/components/ErrorPanel/index.jsx
index c203b88..f77acb5 100644
--- a/src/components/ErrorPanel/index.jsx
+++ b/src/components/ErrorPanel/index.jsx
@@ -15,54 +15,61 @@ import Markdown from '../Markdown';
 import palette from '../../utils/palette';
 
 @withStyles(
-  theme => ({
-    panel: {
-      marginBottom: 3 * theme.spacing.unit,
-    },
-    paper: {
-      padding: `0 ${2 * theme.spacing.unit}px`,
-      display: 'flex',
-      justifyContent: 'space-between',
-      alignItems: 'flex-start',
-    },
-    // Make sure the markdown doesn't overflow the panel
-    markdownContent: {
-      minWidth: 0,
-      overflow: 'auto',
-    },
-    pad: {
-      paddingTop: 9,
-      paddingBottom: 9,
-    },
-    error: {
-      backgroundColor: theme.palette.error.dark,
-      borderColor: theme.palette.error.light,
-    },
-    warning: {
-      backgroundColor: palette.warning.dark,
-      borderColor: palette.warning.light,
-      '& svg': {
-        fill: palette.warning.contrastText,
+  theme => {
+    const warning = theme.palette.warning || palette.warning;
+
+    return {
+      panel: {
+        marginBottom: 3 * theme.spacing.unit,
+      },
+      paper: {
+        padding: `0 ${2 * theme.spacing.unit}px`,
+        display: 'flex',
+        justifyContent: 'space-between',
+        alignItems: 'flex-start',
+      },
+      // Make sure the markdown doesn't overflow the panel
+      markdownContent: {
+        minWidth: 0,
+        overflow: 'auto',
+      },
+      pad: {
+        paddingTop: 9,
+        paddingBottom: 9,
+      },
+      error: {
+        backgroundColor: theme.palette.error.dark,
+        borderColor: theme.palette.error.light,
+      },
+      warning: {
+        backgroundColor: warning.dark,
+        borderColor: warning.light,
+        '& svg': {
+          fill: warning.contrastText,
+        },
+      },
+      errorText: {
+        color: theme.palette.error.contrastText,
+      },
+      warningText: {
+        color: warning.contrastText,
+        '& code': {
+          color: lighten(warning.contrastText, 0.2),
+          fontWeight: 'bold',
+        },
+        '& a': {
+          color: lighten(warning.contrastText, 0.2),
+        },
+      },
+      disabled: {
+        opacity: 1,
       },
-    },
-    errorText: {
-      color: theme.palette.error.contrastText,
-    },
-    warningText: {
-      color: palette.warning.contrastText,
-      '& code': {
-        color: lighten(palette.warning.contrastText, 0.2),
-        fontWeight: 'bold',
+      heading: {
+        fontSize: theme.typography.pxToRem(15),
+        fontWeight: theme.typography.fontWeightRegular,
       },
-    },
-    disabled: {
-      opacity: 1,
-    },
-    heading: {
-      fontSize: theme.typography.pxToRem(15),
-      fontWeight: theme.typography.fontWeightRegular,
-    },
-  }),
+    };
+  },
   { withTheme: true }
 )
 /**
diff --git a/src/components/Markdown/index.jsx b/src/components/Markdown/index.jsx
index c2ba4a2..614eede 100644
--- a/src/components/Markdown/index.jsx
+++ b/src/components/Markdown/index.jsx
@@ -171,10 +171,6 @@ markdown.use(linkAttributes, {
       padding: `${theme.spacing.unit / 2}px ${3 * theme.spacing.unit}px`,
       margin: `${3 * theme.spacing.unit}px 0`,
     },
-    '& a, & a code': {
-      // Style taken from the Link component
-      color: theme.palette.error.contrastText,
-    },
     '& img': {
       maxWidth: '100%',
     },

The main change is adding const warning = theme.palette.warning || palette.warning; and then substituting it wherever we use warning.

Copy link
Contributor Author

@Shubhamchinda Shubhamchinda Oct 15, 2019

Choose a reason for hiding this comment

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

Okay @helfi92 , I tested the changes and it's working but one thing I want to ask why here ? when it was working in src/components/Markdown/index.jsx ?

Is it because so we can apply these changes to any anchor tag not just limited to anchor tag in MarkDownTextArea ? Am I understanding it correct?

Thanks! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to ask why here ?

I'm not sure I understand the question. Why here what? :)

Copy link
Contributor Author

@Shubhamchinda Shubhamchinda Oct 15, 2019

Choose a reason for hiding this comment

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

I'm not sure I understand the question. Why here what?

I mean the previous changes were working too, but why we made the changes in src/components/ErrorPanel/index.jsx.

Is it because so we can apply these changes to any anchor tag not just limited to anchor tag in MarkDownTextArea ? Am I understanding it correct?

But I think I got it now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @helfi92 , I appreciate it! :)

@helfi92
Copy link
Contributor

helfi92 commented Oct 15, 2019

@Shubhamchinda Did you have some time to look at #49 (comment)? :-)

@Shubhamchinda
Copy link
Contributor Author

@Shubhamchinda Did you have some time to look at #49 (comment)? :-)

Hey @helfi92 , I had been busy in college. I'll take a look right now! :)

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

💯 Thank you so much!

@helfi92 helfi92 changed the title Show link on light theme Fix: show link on light theme Oct 15, 2019
@helfi92 helfi92 merged commit 6b2ca82 into mozilla-frontend-infra:master Oct 15, 2019
@helfi92
Copy link
Contributor

helfi92 commented Oct 15, 2019

🎉 This PR is included in version 2.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants