-
Notifications
You must be signed in to change notification settings - Fork 67
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
Changes from 1 commit
4a206ae
47cbcb1
a55d28b
641dd15
7e0fb5c
7a03d83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 size="full" className="bg-danger-300 my-4"> | ||||||
The content in this container don't have a max width | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: a55d28b |
||||||
</Container> | ||||||
|
||||||
<Container size="xl" className="bg-danger-300 my-4"> | ||||||
The content in this container won't exceed the extra large width. | ||||||
</Container> | ||||||
|
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,66 @@ | ||||||
/* eslint-disable react/require-default-props */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [optional] With a recent update to While this repo is currently using v3 of
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it's not needed. Thank you for catching this. |
||||||
|
||||||
const Container: ContainerType = React.forwardRef<HTMLDivElement, ContainerProps>(({ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I changed it to |
||||||
size, | ||||||
children, | ||||||
...props | ||||||
}, ref) => ( | ||||||
<RBContainer | ||||||
bsPrefix="container" | ||||||
fluid | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Do we need these lines when these properties are overridden by the prop defaults below? I'm more concerned with the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. We don't need this. As information only, because the spread |
||||||
{...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 */ | ||||||
size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl', undefined]), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
/** 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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -12,7 +12,7 @@ export interface IDefaultValue { | |
theme?: string, | ||
direction?: string, | ||
language?: string, | ||
containerWidth?: string, | ||
containerWidth?: ContainerSize, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the |
||
}, | ||
theme?: string, | ||
handleSettingsChange: Function, | ||
|
@@ -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); | ||
|
||
|
There was a problem hiding this comment.
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 amax-width
.I think the bootstrap class
mw-100
does this?There was a problem hiding this comment.
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 thesize
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄