-
Notifications
You must be signed in to change notification settings - Fork 25
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
redirect to domain on click #425
redirect to domain on click #425
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -25,7 +29,7 @@ const AppNotificationsHeader: React.FC<IAppNotificationsHeaderProps> = ({ | |||
}) => { | |||
const isMobile = useIsMobile() | |||
const { dappOrigin } = useContext(W3iContext) | |||
const [dropdownToShow, setDropdownToShow] = useState<string | undefined>() | |||
const url = new URL('https://' + domain) |
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.
Definitely not happy with this. We don't have full URL of the dApp in the metadata from Notify Client.
@devceline Do you think we could do this better? Maybe like matching, the dApp with projects and finding the project object, and getting the URL? Both looking overengineering but we cannot update the Notify Client for this too.
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.
Let's move the projects to a Context. We can get the project list in the AppCard and find the project and use the URL from the project object. That would be a better approach. I wouldn't want to ship this. I'll take a look later
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.
Can we huddle, don't fully understand this here?
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.
Okay let's go with this way since we are already sure that the domain wouldn't be empty as we check. I'd still like to have url from the backend instead of creating it on the UI
@@ -25,7 +29,7 @@ const AppNotificationsHeader: React.FC<IAppNotificationsHeaderProps> = ({ | |||
}) => { | |||
const isMobile = useIsMobile() | |||
const { dappOrigin } = useContext(W3iContext) | |||
const [dropdownToShow, setDropdownToShow] = useState<string | undefined>() | |||
const url = new URL('https://' + domain) |
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.
Can we huddle, don't fully understand this here?
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.
LGTM
Description
On the topic details page header, make the URL clickable.
Type of change
Checklist:
Additional Information (Optional)
Slack thread