Skip to content

Page Layouts - Added data collection layout #4652

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

Merged
merged 12 commits into from
Jan 10, 2025
Merged

Page Layouts - Added data collection layout #4652

merged 12 commits into from
Jan 10, 2025

Conversation

halocline
Copy link
Collaborator

@halocline halocline commented Jan 10, 2025

Deploy Preview

What does this PR do?

  • Adds data collection page layout to grommet demo app.

Where should the reviewer start?

  • sandbox/grommet-app/pages/layouts.index.tsx

What testing has been done on this PR?

In addition to the feature you are implementing, have you checked the following:

General UX Checks

  • Small, medium, and large screen sizes
  • Cross-browsers (FireFox, Chrome, and Safari)
  • Light & dark modes
  • All hyperlinks route properly

Accessibility Checks

  • Keyboard interactions
  • Screen reader experience
  • Run WAVE accessibility plugin (Chrome)

Code Quality Checks

  • Console is free of warnings and errors
  • Passes E2E commit checks
  • Visual snapshots are reasonable

How should this be manually tested?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Should this PR be mentioned in Design System updates?

Is this change backwards compatible or is it a breaking change?

Copy link

changeset-bot bot commented Jan 10, 2025

⚠️ No Changeset found

Latest commit: a407a05

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for unrivaled-bublanina-3a9bae ready!

Name Link
🔨 Latest commit a407a05
🔍 Latest deploy log https://app.netlify.com/sites/unrivaled-bublanina-3a9bae/deploys/6781a1b84ca78a0008979bba
😎 Deploy Preview https://deploy-preview-4652--unrivaled-bublanina-3a9bae.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.

@halocline halocline requested a review from taysea January 10, 2025 19:22

export const RoutedAnchor = ({ ...rest }: RoutedAnchorProps) => (
<Anchor {...rest} />
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
);
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1 @@
export * from './RoutedAnchor';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export * from './RoutedAnchor';
export * from './RoutedAnchor';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1 @@
export * from './Region';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export * from './Region';
export * from './Region';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

<PageHeader title="Layouts" />
<List data={layouts} defaultItemProps={{ pad: { vertical: 'xxsmall' } }}>
{(item) => (
<RoutedAnchor key={item} as={Link} to={`/layouts/${item.toLowerCase()}`} label={item} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<RoutedAnchor key={item} as={Link} to={`/layouts/${item.toLowerCase()}`} label={item} />
<RoutedAnchor key={item} alignSelf="start" as={Link} to={`/layouts/${item.toLowerCase()}`} label={item} />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

<Route key="dashboard" path="/layouts/dashboard" element={<div>Dashboard</div>} />
]

export { Layouts, routes };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export { Layouts, routes };
export { Layouts, routes };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

</PageContent>
</Page >
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

<Menu label="Actions" items={tableActions} />
</Box>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

/>
</Data>
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
};
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

/>
</Box>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1 @@
export * from './Collection';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export * from './Collection';
export * from './Collection';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

{ property: 'type', header: 'Type' },
];

export { privateCloudColumns, publicCloudColumns };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export { privateCloudColumns, publicCloudColumns };
export { privateCloudColumns, publicCloudColumns };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

})}
</Box >
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

onActive={index => setCloudType(cloudTypes[index])}
>
<Tab title="Private cloud">
<Grid columns={['auto', 'flex']} gap="large" pad={{ top: 'medium' }} align="start">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Soft opinion/refinement -- should we set a max-width on the left column in case the menu labels ever gotten too long?

Suggested change
<Grid columns={['auto', 'flex']} gap="large" pad={{ top: 'medium' }} align="start">
<Grid columns={[['auto', 'medium'], 'flex']} gap="large" pad={{ top: 'medium' }} align="start">

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see now that the CollectionMenu is already setting its own min/max, so feel free to ignore this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I might come back to this. It'd be nice to have the Grid fully in charge, but when I did [['auto', 'medium'], ...] it was always growing to medium because the table had room to flex and the menu width just felt excessive.

If only we had greater granularity in our t-shirt sizes... 🤪

skeleton={undefined}
round="small"
>
<CollectionMenu items={dataSets[cloudType]} selected={dataSet} onSelect={setDataSet} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refinement -- seems like we have this repeated here and line 105. Should we pull it out into a variable to simplify any logic changes in future

const collectionMenu = <CollectionMenu .../>

...
<ContentPane ...>
  {collectionMenu}
</ContentPane>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@halocline halocline requested a review from taysea January 10, 2025 22:42
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

Thank you! This will be helpful.

@halocline halocline merged commit f9c370c into master Jan 10, 2025
9 of 10 checks passed
@halocline halocline deleted the layout-grids branch January 10, 2025 23:35
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