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

PORTALS-3312: New Portal feature highlights component #1437

Merged
merged 17 commits into from
Dec 13, 2024

Conversation

kianamcc
Copy link
Collaborator

@kianamcc kianamcc commented Dec 4, 2024

Comment on lines 32 to 37
<Typography
component="span"
sx={{ fontWeight: 'bold', color: 'primary.main' }}
>
CAVATICA
</Typography>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be a link?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed with Adam this should link to the Cavatica website

Comment on lines 24 to 25
import analyzetheclouds from '@sage-bionetworks/synapse-portal-framework/components/elportal/assets/analyzetheclouds.png'
import computationaltools from '@sage-bionetworks/synapse-portal-framework/components/elportal/assets/computationaltools.png'
Copy link
Collaborator

Choose a reason for hiding this comment

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

move these assets to the elportal project if they are only used in the elportal

Copy link
Collaborator

@nickgros nickgros Dec 6, 2024

Choose a reason for hiding this comment

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

I am a bit curious about how these imports work. I would expect Vite would need you to have the ?url suffix, but maybe what you have is fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been working for me! I assume Vite is configured to handle image assets correctly?

@kianamcc kianamcc added the chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests label Dec 13, 2024
Copy link
Collaborator

@nickgros nickgros left a comment

Choose a reason for hiding this comment

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

Looks great

Comment on lines +29 to +32
gridTemplateAreas: {
xs: `'image' 'content'`,
lg: reverseOrder ? `'content image'` : `'image content'`,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice use of gridTemplateAreas, it's very clear how the layout is switching between sizes and reverseOrder state

@kianamcc kianamcc merged commit e8edec1 into Sage-Bionetworks:main Dec 13, 2024
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants