Skip to content

feat: add Learn path, change menu icons to use the new design#32

Open
scorebreaker wants to merge 2 commits into
opendexnetwork:mainfrom
scorebreaker:add-learn
Open

feat: add Learn path, change menu icons to use the new design#32
scorebreaker wants to merge 2 commits into
opendexnetwork:mainfrom
scorebreaker:add-learn

Conversation

@scorebreaker

@scorebreaker scorebreaker commented Mar 30, 2021

Copy link
Copy Markdown
Contributor

Add Learn path, change menu icons to use the new design. Also, I decided to use inline svg-s for the menu icons, because otherwise I would have needed to use hacky ways to get the hover working (https://css-tricks.com/the-many-ways-to-change-an-svg-fill-on-hover-and-when-to-use-them/).

@stackingcats stackingcats left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you. Looks good in general.
A few concerns:

  • Would it be worth the effort to dynamically fetch the videos instead of hard-coding them?
  • When navigating to the learn view, I get an error
    Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘https://www.youtube.com’) does not match the recipient window’s origin (‘https://localhost:3000’).
    and lots of warnings
    Some cookies are misusing the recommended “SameSite“ attribute
    Please let's wipe those off.

Comment thread src/stories/ConsoleIcon.stories.tsx Outdated
import { Story, Meta } from "@storybook/react/types-6-0";

export default {
title: "ConsoleIcon",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe would make sense to group all the menu icons under one story?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@scorebreaker

scorebreaker commented Apr 7, 2021

Copy link
Copy Markdown
Contributor Author
  • Would it be worth the effort to dynamically fetch the videos instead of hard-coding them?

Done.

  • When navigating to the learn view, I get an error
    Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘https://www.youtube.com’) does not match the recipient window’s origin (‘https://localhost:3000’).
    and lots of warnings
    Some cookies are misusing the recommended “SameSite“ attribute
    Please let's wipe those off.

I couldn't find solution to Failed to execute ‘postMessage’ on ‘DOMWindow’ issue, I checked that it is related with youtube iframe get call (tjallingt/react-youtube#96), but not caused by react-youtube library. Also, "Some cookies are misusing the recommended “SameSite“ attribute" is still an issue as well - from what I researched, cookies are controlled from server-side and youtube should take care of cookies itself.

@stackingcats stackingcats requested a review from maybeast April 8, 2021 08:36
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.

2 participants