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

Add set membership columns to the element view table #435

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

NateLanza
Copy link
Contributor

@NateLanza NateLanza commented Dec 10, 2024

Does this PR close any open issues?

Closes #399

Give a longer description of what this PR addresses and why it's needed

Adds boolean columns (showing X or ✓) for each set to the query result table, indicating the set membership of each element.

Provide pictures/videos of the behavior before and after these changes (optional)

Before:
309-before
After:
309-after

Have you added or updated relevant tests?

  • Yes
  • No changes are needed

Have you added or updated relevant documentation?

  • Yes
  • No changes are needed

@NateLanza NateLanza self-assigned this Dec 10, 2024
Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit c49f9c0
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/6759ee25bf08b800082d7e91
😎 Deploy Preview https://deploy-preview-435--upset2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NateLanza NateLanza marked this pull request as ready for review December 10, 2024 23:15
@@ -40,10 +41,16 @@ function useColumns(columns: string[]) {
* Table to display elements
*/
export const ElementTable: FC = () => {
const attributeColumns = useRecoilValue(attributeAtom);
const attColumns = useRecoilValue(attributeAtom);
Copy link
Member

Choose a reason for hiding this comment

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

I prefer that this is fully spelled out. Makes things more readable (for me who never works in the code)

Copy link
Member

Choose a reason for hiding this comment

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

Agree with jack here

const columns = useColumns(['_label', ...attributeColumns]);
const setColumns = useRecoilValue(setColumnsSelector);
let columns = useColumns(['_label', ...([...attColumns, ...setColumns].filter((col) => !col.startsWith('_')))]);
// Sort set columns to the right of other columns & add a boolean type to
Copy link
Member

Choose a reason for hiding this comment

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

Does this sort? Looks like it just adds type

Copy link
Member

@JakeWags JakeWags Dec 11, 2024

Choose a reason for hiding this comment

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

The "sorting" here is really just the spread operator, putting the setColumns at the end of the list.
@JackWilb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that comment is outdated; there used to be an explicit sort function but that was removed when I started using spread instead. Comment updated.

Copy link
Member

@JakeWags JakeWags left a comment

Choose a reason for hiding this comment

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

Update attribute variable name. Otherwise looks great.

@@ -40,10 +41,16 @@ function useColumns(columns: string[]) {
* Table to display elements
*/
export const ElementTable: FC = () => {
const attributeColumns = useRecoilValue(attributeAtom);
const attColumns = useRecoilValue(attributeAtom);
Copy link
Member

Choose a reason for hiding this comment

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

Agree with jack here

const columns = useColumns(['_label', ...attributeColumns]);
const setColumns = useRecoilValue(setColumnsSelector);
let columns = useColumns(['_label', ...([...attColumns, ...setColumns].filter((col) => !col.startsWith('_')))]);
// Sort set columns to the right of other columns & add a boolean type to
Copy link
Member

@JakeWags JakeWags Dec 11, 2024

Choose a reason for hiding this comment

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

The "sorting" here is really just the spread operator, putting the setColumns at the end of the list.
@JackWilb

@NateLanza NateLanza merged commit e84f782 into main Dec 12, 2024
7 checks passed
@NateLanza NateLanza deleted the 309-query-table-incl-sets branch December 12, 2024 19:33
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.

Show set membership in query result table
3 participants