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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Sep 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a206ae
feat: allow Container render without a max width
rpenido Sep 5, 2024
47cbcb1
docs: update size property api docs
rpenido Sep 6, 2024
a55d28b
fix: props override and docs
rpenido Sep 6, 2024
641dd15
fix: remove deprecated from ContainerType
rpenido Sep 9, 2024
7e0fb5c
fix: change forwardRef from HTMLDivElement to Element
rpenido Sep 9, 2024
7a03d83
fix: remove unecessary undefined prop value
rpenido Sep 9, 2024
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
22 changes: 14 additions & 8 deletions src/Container/Container.test.jsx → src/Container/Container.test.tsx
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

64 changes: 64 additions & 0 deletions src/Container/index.tsx
View file
Open in desktop
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>;

const Container: ContainerType = React.forwardRef<Element, ContainerProps>(({
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']),
/** 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
View file
Open in desktop
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
View file
Open in desktop
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