| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 78
feat: remove fonts from package, update ActionBar, Button and InfoLabel #908
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
Conversation
jbadan
changed the title
| <style type="text/css"> | ||
| @font-face { | ||
| font-family: '72'; | ||
| src: url('./72-Regular.woff') format('woff'); |
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.
This is stupid and annoying, but I couldn't get the fonts from @sap-theming to load from node_modules in here 😢
|
Deploy preview for fundamental-react ready! Built with commit 508f3a3 |
| fs.copyFile('README.md', 'src/_playground/documentation/Home/README.md', (err) => { | ||
| if (err) throw err; | ||
| }); | ||
|
|
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.
moved to scripts/copy-assets.js
| buttonContainerClassNames: 'Classnames to spread to the back Button container.', | ||
| description: 'Localized text for the description.', | ||
| descriptionProps: 'Additional props to be spread to the description\'s `<p>` element.', | ||
| headingLevel: 'Heading level. `<h1>` is reserved for the page title.', |
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.
if h1 is reserved, should the propType be CustomPropTypes.range(2, 6), then?
src/ActionBar/ActionBar.js
Outdated
| actionClassNames: 'Classnames to spread to the action Button container.', | ||
| actionProps: 'Props to spread to the action Button container', | ||
| actions: 'Button components to add to the ActionBar.', | ||
| buttonContainerClassNames: 'Classnames to spread to the back Button container.', |
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.
Seems weird to have actionClassNames and buttonContainerClassNames plural when we use className which isn't plural for the same idea.
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.
Looks like there is a mixture of both ways of doing it in fundamental-react
actionClassNames
buttonContainerClassNames
labelClassNames
inputClassNames
listClassNames
tokenizerClassNames
vs
backdropClassName
contentClassName
inputClassName
popperClassName
tableBodyClassName
table....ClassName
referenceClassName
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 think that came from a bunch of different people implementing it. We should do a separate story to clean it up in one go.
I'll make this one singular for now.
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.
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.
Now that I look at the history on it.... it's only me that's been adding the plural ones 😆🤣
src/ActionBar/ActionBar.js
Outdated
| description: 'Localized text for the description.', | ||
| descriptionProps: 'Additional props to be spread to the description\'s `<p>` element.', | ||
| headingLevel: 'Heading level. `<h1>` is reserved for the page title.', | ||
| onClick: 'Callback to pass to the back Button container.' |
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.
isn't it passed to the button and not its container?
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.
There is no subcomponent anymore. That said, should we name this prop something to reflect that it's for the back button (e.g. onBackClick)?
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.
LGTM
package.json
Outdated
| "start:playground": "npm run storybook", | ||
| "std-version": "standard-version -m \"chore(release): version %s build ${TRAVIS_BUILD_NUMBER} [ci skip]\"", | ||
| "storybook": "start-storybook -p 12123", | ||
| "storybook": "node scripts/copy-assets.js && start-storybook -p 12123 -s src/_playground/fonts", |
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.
Would we want to make the static directory more generic and then just include a fonts directory within it?
src/ActionBar/ActionBar.js
Outdated
| description: 'Localized text for the description.', | ||
| descriptionProps: 'Additional props to be spread to the description\'s `<p>` element.', | ||
| headingLevel: 'Heading level. `<h1>` is reserved for the page title.', | ||
| onClick: 'Callback to pass to the back Button container.' |
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.
There is no subcomponent anymore. That said, should we name this prop something to reflect that it's for the back button (e.g. onBackClick)?
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.
Looks good. 🚢
Description
Badgefolder toCounterfolder - import asimport { Counter } from 'fundamental-react/Counter'option=‘light’tooption=‘transparent’