PORTALS-3308 - What's in Portals Component (GoalsV2) #1399
+644
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
afwillia
changed the title
afwillia
changed the title
nickgros
added
the
chromatic
nickgros
requested changes
Dec 4, 2024
Comment on lines
+159
to
+181
| <Link | ||
| href="https://eliteportal.synapse.org/Explore/Data" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| fontFamily="Lato" | ||
| fontSize="18" | ||
| fontStyle="semi-bold" | ||
| > | ||
| <Button | ||
| variant="contained" | ||
| sx={{ | ||
| backgroundColor: '#5BA998', | ||
| border: '1px solid white', | ||
| boxShadow: 'none', | ||
| '&:hover': { | ||
| backgroundColor: darken('#5BA998', 0.05), | ||
| boxShadow: 'none', | ||
| }, | ||
| }} | ||
| > | ||
| Start Exploring Data | ||
| </Button> | ||
| </Link> |
There was a problem hiding this comment.
You should be able to apply the relevant Link props to the Button component. Also, the font/size changes probably shouldn't be necessary to apply to the button
Suggested change
| <Link | |
| href="https://eliteportal.synapse.org/Explore/Data" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| fontFamily="Lato" | |
| fontSize="18" | |
| fontStyle="semi-bold" | |
| > | |
| <Button | |
| variant="contained" | |
| sx={{ | |
| backgroundColor: '#5BA998', | |
| border: '1px solid white', | |
| boxShadow: 'none', | |
| '&:hover': { | |
| backgroundColor: darken('#5BA998', 0.05), | |
| boxShadow: 'none', | |
| }, | |
| }} | |
| > | |
| Start Exploring Data | |
| </Button> | |
| </Link> | |
| <Button | |
| href="https://eliteportal.synapse.org/Explore/Data" // <-- this should probably be a prop for reusability | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| variant="contained" | |
| sx={{ | |
| backgroundColor: '#5BA998', | |
| border: '1px solid white', | |
| boxShadow: 'none', | |
| '&:hover': { | |
| backgroundColor: darken('#5BA998', 0.05), | |
| boxShadow: 'none', | |
| }, | |
| }} | |
| > | |
| Start Exploring Data | |
| </Button> |
Comment on lines
+185
to
+211
| {queryResultBundle?.queryResult!.queryResults.rows.map((el, index) => { | ||
| const values = el.values as string[] | ||
| if (values.some(value => value === null)) { | ||
| // We cast values above assuming there are no null values, emit a warning just in case. | ||
| console.warn('Row has null value(s) when no nulls expected') | ||
| } | ||
| const tableId = | ||
| tableIdColumnIndex > -1 ? values[tableIdColumnIndex] : undefined | ||
| let countSql | ||
| if (countSqlColumnIndex > -1 && values[countSqlColumnIndex]) { | ||
| countSql = values[countSqlColumnIndex] | ||
| } else if (tableId) { | ||
| countSql = `SELECT * FROM ${tableId}` | ||
| } | ||
| const title = values[titleColumnIndex] | ||
| const summary = values[summaryColumnIndex] | ||
| const link = values[linkColumnIndex] | ||
| // assume that we recieve assets in order of rows and there is an asset for each item | ||
| // can revisit if this isn't the case. | ||
| const asset = assets?.[index] ?? '' | ||
| const goalsV2DataProps: GoalsV2DataProps = { | ||
| countSql, | ||
| title, | ||
| summary, | ||
| link, | ||
| asset, | ||
| } |
There was a problem hiding this comment.
This is a lot of logic in the returned JSX. Suggest creating an array of goalsV2DataProps outside of the return and then calling goalsV2DataPropsArray.map here
| padding-right: 25px; | ||
| } | ||
|
|
||
| .GoalsV2 { |
There was a problem hiding this comment.
There is a lot of duplicate styling between GoalsV1 and GoalsV2, and it's a bit confusing because there is a lot of styling happening both here and in the component file. Can you either:
- Refactor to share styles with GoalsV1 and minimize the duplicates, and put style overrides here instead of in the component
- Remove most/all of these styles and for GoalsV2 just use MUI builtin components and sx styles
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PORTALS-3308 This PR modifies the Goals component by creating cards for each panel, surrounded by a box.