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 Scrollbox container component #170

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Add Scrollbox container component #170

merged 1 commit into from
Aug 11, 2021

Conversation

lyzadanger
Copy link
Contributor

This PR adds a Scrollbox container component based on the scrollbox pattern. It also extracts a partial pattern for a sticky-header, as inspired by this comment in a previous PR. The only place a sticky header is currently used in our applications is in a table, so I haven't fully fleshed out this pattern, but it exists as a stub and the table-heading styling makes use of it now.

This Scrollbox component will be used by forthcoming Table component.

You can see this component in action on the Container Components pattern-library page.

Depends on #169 as it builds on a pattern-library page modified in that branch.

image

@lyzadanger lyzadanger requested a review from esanzgar August 10, 2021 17:52
Copy link
Contributor

@esanzgar esanzgar left a comment

Choose a reason for hiding this comment

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

I added a few comments.

I would suggest to use the new <Scrollbox> component in the LongModalExample (DialogComponents.js):

        <Scrollbox>
          <LoremIpsum />
        </Scrollbox>

src/components/containers.js Show resolved Hide resolved
it('renders children inside of a div with appropriate classnames', () => {
const wrapper = createComponent();

assert.isTrue(wrapper.find('div').first().hasClass('Hyp-Scrollbox'));
Copy link
Contributor

@esanzgar esanzgar Aug 11, 2021

Choose a reason for hiding this comment

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

More consistent with the test below:

Suggested change
assert.isTrue(wrapper.find('div').first().hasClass('Hyp-Scrollbox'));
assert.isTrue(wrapper.find('div.Hyp-Scrollbox').exists());

it('applies extra classes', () => {
const wrapper = createComponent({ classes: 'foo bar' });

assert.isTrue(wrapper.find('div.Hyp-Scrollbox.foo.bar').exists());
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to check the correct class order, you can use this complicated test:

Suggested change
assert.isTrue(wrapper.find('div.Hyp-Scrollbox.foo.bar').exists());
assert.deepEqual(
[
...wrapper
.find('div.Hyp-Scrollbox.foo.bar')
.getDOMNode()
.classList.values(),
],
['Hyp-Scrollbox', 'foo', 'bar' ]
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note to #129 that could help to address this: I'd like to craft some common/reusable tests for components that take these common props.

* A sticky container that is sized one-touch-unit high. This is currently
* expanded upon by table styling, but not used in other patterns (yet).
*/
@mixin sticky-header {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@mixin sticky-header {
@mixin sticky-scrollbox-header {

The association of the two names could help to understand when to use the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right to note that this pattern isn't quite normalized where we need it yet. I spent a few minutes trying to see if I could re-jigger it (also taking the comment below about color into consideration), but the pattern lacks clarity out of context. What I'd like to propose is that we carry this forward into the next set of work, which involves a Table component that uses the Scrollbox component as a wrapper. Then we can scrutinize this pattern in better context and make some improvements.

@include layout.padding;
@include atoms.border(bottom);

background-color: var.$color-grey-1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the colour could be taken out of this pattern, so the pattern only deals about positioning and size.

@lyzadanger lyzadanger force-pushed the scrollbox-component branch from 6690d3c to 56fad4e Compare August 11, 2021 12:53
Base automatically changed from pattern-fixtures to main August 11, 2021 12:53
@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #170 (83090c8) into main (99f304c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #170   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          149       152    +3     
  Branches        51        53    +2     
=========================================
+ Hits           149       152    +3     
Impacted Files Coverage Δ
src/components/containers.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99f304c...83090c8. Read the comment docs.

@lyzadanger lyzadanger force-pushed the scrollbox-component branch from 56fad4e to 83090c8 Compare August 11, 2021 13:20
@lyzadanger lyzadanger merged commit 42b72f6 into main Aug 11, 2021
@lyzadanger lyzadanger deleted the scrollbox-component branch August 11, 2021 13:23
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