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

feat: migrates Container to TypeScript; Container without max-width on docs site [FC-0062] #3216

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React from 'react';
import { render } from '@testing-library/react';
import Container from '.';
import Container, { type ContainerSize } from '.';

const getClassNames = (container) => container.className.split(' ');
const getClassNames = (container: HTMLElement) => container.className.split(' ');

describe('<Container />', () => {
it('displays children', () => {
Expand All @@ -12,32 +12,38 @@ describe('<Container />', () => {

it('adds the .container-fluid class', () => {
const { container } = render(<Container>Content</Container>);
const containerElement = container.firstChild;
const containerElement = container.firstChild as HTMLElement;
expect(getClassNames(containerElement)).toContain('container-fluid');
});

it('adds the .container class', () => {
const { container } = render(<Container fluid={false}>Content</Container>);
const containerElement = container.firstChild;
expect(getClassNames(containerElement)).toContain('container');
const containerElement = container.firstChild as HTMLElement;
expect(getClassNames(containerElement!)).toContain('container');
expect(getClassNames(containerElement)).not.toContain('container-fluid');
});

['xs', 'sm', 'md', 'lg', 'xl'].forEach((size) => {
['xs', 'sm', 'md', 'lg', 'xl'].forEach((size: ContainerSize) => {
it(`adds the .container-mw-${size} class`, () => {
const { container } = render(<Container size={size}>Content</Container>);
const containerElement = container.firstChild;
const containerElement = container.firstChild as HTMLElement;
expect(getClassNames(containerElement)).toContain(`container-mw-${size}`);
});
});

it('does not add a size class when size is not specified', () => {
const { container } = render(<Container>Content</Container>);
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather use an explicit size="full" than assume that omitting the size means "full width".. Omitting a size should make the container fit its contents, just like a <div> without a max-width.
I think the bootstrap class mw-100 does this?

Copy link
Contributor Author

@rpenido rpenido Sep 6, 2024

Choose a reason for hiding this comment

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

I agree with you and tried this approach first.

The problem is that the current component allows the size props to be omitted, and it behaves like "full width". You can check it here, removing the size from one example: https://paragon-openedx.netlify.app/components/container/

We have many containers in our code base without size that probably rely on this, so I don't think we can change that.
So I pushed back the size={full} option.

We can add both options, but I think the ambiguity will not help us here.

Copy link
Contributor Author

@rpenido rpenido Sep 6, 2024

Choose a reason for hiding this comment

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

I tried to update the API docs here: 47cbcb1

Copy link
Contributor

@pomegranited pomegranited Sep 10, 2024

Choose a reason for hiding this comment

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

The problem is that the current component allows the size props to be omitted, and it behaves like "full width". You can check it here, removing the size from one example: https://paragon-openedx.netlify.app/components/container/

Passing in size="" gives us the fullscreen behaviour you're adding here.. so do we need this change?

Edit: ah yes, this is what Adam said 😄

const containerElement = container.firstChild as HTMLElement;
expect(getClassNames(containerElement)).toEqual(['container-fluid']);
});

it('preserves custom class names', () => {
const { container } = render(
<Container className="custom-class" size="md">
Content
</Container>,
);
const containerElement = container.firstChild;
const containerElement = container.firstChild as HTMLElement;
expect(getClassNames(containerElement)).toContain('container-mw-md');
expect(getClassNames(containerElement)).toContain('container-fluid');
expect(getClassNames(containerElement)).toContain('custom-class');
Expand Down
4 changes: 4 additions & 0 deletions src/Container/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ The base container to contain, pad, and center content in the viewport. This com
```jsx live
<div style={{ overflowX: 'auto' }}>
<div style={{ width: '1500px', border: 'solid 3px red' }}>
<Container className="bg-danger-300 my-4">
The content in this container doesn't have a max width
</Container>
<Container size="xl" className="bg-danger-300 my-4">
The content in this container won't exceed the extra large width.
</Container>
Expand Down
49 changes: 0 additions & 49 deletions src/Container/index.jsx

This file was deleted.

64 changes: 64 additions & 0 deletions src/Container/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/* eslint-disable react/require-default-props */
Copy link
Member

Choose a reason for hiding this comment

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

[optional] With a recent update to @edx/eslint-config, we might be able to upgrade to v4.2.0 in this repo and remove the need for default params in this file without disabling the ESLint rule 👀 openedx/eslint-config#156

While this repo is currently using v3 of @edx/eslint-config, the v4 breaking changes don't seem to apply to this repo at the moment, per the release notes, so the upgrade should be fairly trivial:

Drops support for eslint ^6.8.0; @edx/eslint-config must now be used with at least eslint versions ^7.32.0 or ^8.2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamstankiewicz I tried to update but got the following error on linting (which prevents the commit).
Did you already experienced this error before?

> @openedx/[email protected] lint
> npm run stylelint && eslint --ext .js --ext .jsx --ext .ts --ext .tsx . && npm run lint --workspaces --if-present


> @openedx/[email protected] stylelint
> stylelint "src/**/*.scss" "scss/**/*.scss" "www/src/**/*.scss" --config .stylelintrc.json


Oops! Something went wrong! :(

ESLint: 8.18.0

Error: Error while loading rule '@typescript-eslint/naming-convention': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
Occurred while linting /home/rpenido/Projects/opencraft/paragon/.eslintrc.js
...

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I have not. Thanks for flagging. I am able to reproduce the issue on my end as well. Unless you want to investigate the error further, feel free to defer on the upgrade and continue with your existing solution using default props and the disabled ESLint rule. If you do defer on the investigation/resolution, do you mind filing a new Paragon issue to document the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created: #3230

import React from 'react';
import classNames from 'classnames';
import PropTypes from 'prop-types';
import RBContainer, { type ContainerProps as RBContainerProps } from 'react-bootstrap/Container';

import type { ComponentWithAsProp } from '../utils/types/bootstrap';

enum ContainerSizeClass {
xs = 'container-mw-xs',
sm = 'container-mw-sm',
md = 'container-mw-md',
lg = 'container-mw-lg',
xl = 'container-mw-xl',
}

export type ContainerSize = keyof typeof ContainerSizeClass;

interface ContainerProps extends RBContainerProps {
size?: ContainerSize;
}

type ContainerType = ComponentWithAsProp<'div', ContainerProps> & { Deprecated?: any };
Copy link
Member

Choose a reason for hiding this comment

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

Is the Deprecated property necessary here? Container doesn't have a deprecated sub-component (e.g., Container.Deprecated) as Button.Deprecated does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not needed. Thank you for catching this.
Fixed here: 641dd15


const Container: ContainerType = React.forwardRef<HTMLDivElement, ContainerProps>(({
Copy link
Member

Choose a reason for hiding this comment

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

Could Container be used with a non-div element, e.g. with as="span"? I wonder if this should be more generic, e.g. HTMLElement or even Element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I changed it to Element as our property of interest (className) exists.
Here: 7e0fb5c

size,
children,
...props
}, ref) => (
<RBContainer
{...props}
ref={ref}
className={classNames(
props.className,
size && ContainerSizeClass[size],
)}
>
{children}
</RBContainer>
));

Container.propTypes = {
...RBContainer.propTypes,
/** Override the base element */
as: PropTypes.elementType,
/** Specifies the contents of the container */
children: PropTypes.node,
/** Fill all available space at any breakpoint */
fluid: PropTypes.bool,
/** Set the maximum width for the container. Omiting the prop will remove the max-width */
size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl', undefined]),
Copy link
Member

Choose a reason for hiding this comment

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

I believe the undefined is implied by the lack of .isRequired on the prop type definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. It was there to show in the docs, but it isn't necessary with the docstring change.
Fixed: 7a03d83

/** Overrides underlying component base CSS class name */
bsPrefix: PropTypes.string,
};

Container.defaultProps = {
as: 'div',
children: undefined,
fluid: true,
size: undefined,
bsPrefix: 'container',
};

export default Container;
2 changes: 1 addition & 1 deletion src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export { default as Bubble } from './Bubble';
export { default as Button, ButtonGroup, ButtonToolbar } from './Button';
export { default as Chip, CHIP_PGN_CLASS } from './Chip';
export { default as ChipCarousel } from './ChipCarousel';
export { default as Container, ContainerSize } from './Container';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
export { default as Icon } from './Icon';
export { default as IconButton, IconButtonWithTooltip } from './IconButton';
Expand Down Expand Up @@ -41,7 +42,6 @@ export const
export const CheckBox: any; // from './CheckBox';
export const CheckBoxGroup: any; // from './CheckBoxGroup';
export const CloseButton: any; // from './CloseButton';
export const Container: any; // from './Container';
export const Layout: any, Col: any, Row: any; // from './Layout';
export const Collapse: any; // from './Collapse';
export const Collapsible: any; // from './Collapsible';
Expand Down
6 changes: 3 additions & 3 deletions www/src/context/SettingsContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { createContext, useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import { Helmet } from 'react-helmet';
import { IntlProvider } from 'react-intl';
import { messages } from '~paragon-react';
import { messages, type ContainerSize } from '~paragon-react';

import { THEMES, DEFAULT_THEME } from '../../theme-config';
import { SETTINGS_EVENTS, sendUserAnalyticsEvent } from '../../segment-events';
Expand All @@ -12,7 +12,7 @@ export interface IDefaultValue {
theme?: string,
direction?: string,
language?: string,
containerWidth?: string,
containerWidth?: ContainerSize,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the ContainerSize type here, too! 🙌

},
theme?: string,
handleSettingsChange: Function,
Expand All @@ -35,7 +35,7 @@ function SettingsContextProvider({ children }) {
theme: DEFAULT_THEME,
direction: 'ltr',
language: 'en',
containerWidth: 'md',
containerWidth: 'md' as ContainerSize,
});
const [showSettings, setShowSettings] = useState(false);

Expand Down
Loading