Skip to content
This repository has been archived by the owner on May 7, 2019. It is now read-only.

SCM-762 #101

Merged
merged 84 commits into from
Nov 2, 2017
Merged

SCM-762 #101

merged 84 commits into from
Nov 2, 2017

Conversation

mean2me
Copy link
Contributor

@mean2me mean2me commented Oct 20, 2017

No description provided.

* added react-dom to "externals" in webpack.config.js
* removed from repository a unused bundle file
* added a missing dev dependency (eslint-config-dnn)
…pack dependency for SvgIcons common component
…pack dependency for Checkbox common component
…pack dependency for Collapsible common component
…pack dependency for CollapsibleRow common component
…pack dependency for ContentLoaderWrapper common component
…pack dependency for DatePicker common component
…pack dependency for DayPicker common component
…pack dependency for DatePicker common component
…pack dependency for Dropdown common component
…pack dependency for DropdownWithError common component
…pack dependency for Droppable common component
…pack dependency for EditableField common component
…pack dependency for FileUpload common component
…pack dependency for FolderPicker common component
…pack dependency for GridCell common component
…pack dependency for GridSystem common component
…pack dependency for InputGroup common component
…pack dependency for MultilineInput common component
…pack dependency for MultiLineInputWithError common component
…CM-762-merge

# Conflicts:
#	Collapsible/lib/Collapsible.js
#	FileUpload/lib/FileUpload.js
#	FileUpload/package.json
#	InputGroup/lib/InputGroup.js
#	InputGroup/package.json
#	PagePicker/lib/PagePicker.js
#	PagePicker/package.json
#	PersonaBarPageHeader/lib/PersonaBarPageHeader.js
#	PersonaBarPageHeader/package.json
#	PersonaBarPageHeader/webpack.config.js
#	SvgIcons/lib/SvgIcons.js
#	SvgIcons/package.json
#	SvgIcons/src/SvgIcons.jsx
#	TextOverflowWrapper/package.json
#	TextOverflowWrapper/webpack.config.js
#	TextOverflowWrapperNew/lib/TextOverflowWrapperNew.js
#	TextOverflowWrapperNew/package.json
#	Tooltip/lib/Tooltip.js
node_modules
/sinopiautil.js
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,6 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

How come the components before this one (alphabetically) didn't have a .babelrc?

"react": "^15.3.2",
"react-day-picker": "^2.3.3",
"moment": "2.13.0",
"raw-loader": "0.5.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed !raw! in jsx so maybe this dependency is no longer needed?

"url-loader": "0.5.7",
"webpack": "1.13.0"
"react": "^15.6.2",
"react-custom-scrollbars": "^4.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, are we using more than one solution for scrollbars?

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 also found different scrollbars' libraries in other projects.

fileInput.value = '';
clearFileUploaderValue(fileInput) {
if (fileInput) {
fileInput.value = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I though single quotes were the default ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

project standard seems to be double quotes :)

"react": "^15.3.2",
"dnn-global-styles": "0.0.5",
"dnn-grid-cell": "0.0.4",
"dnn-back-to-link": "0.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

How cute. These dnn components weren't even included in old dependencies :D

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 had to check deps one by one... it happened in many other places and made testing and debugging quite hard before.

@@ -1,37 +1,49 @@
{
"name": "dnn-persona-bar-page-header",
"version": "0.0.29",
"version": "0.0.31",
Copy link
Contributor

Choose a reason for hiding this comment

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

I keep looking at those versions and I'm getting the impression that we're not really using server correctly?

@@ -17,6 +17,7 @@
"babel-loader": "6.2.4",
"babel-preset-es2015": "6.6.0",
"babel-preset-react": "6.5.0",
"cpy": "^6.0.0",
"css-loader": "0.23.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file even here, in root? And package name is dnn-tree-control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe some wild cut and paste :)

"dnn-text-overflow-wrapper-new": "0.0.4",
"shortid": "^2.2.8"
"shortid": "^2.2.8",
"lodash": "4.17.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing tree shaking in end apps? If not maybe we could import the individual features of lodash?

@@ -4,8 +4,6 @@ const searchIcon = require("!raw!./img/search.svg");
const fileIcon = require("!raw!./img/pages.svg");
import { Scrollbars } from "react-custom-scrollbars";

const notSpecifiedText = "<None Specified>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, where is the default text now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the real problem here is the "!raw!" prefix... I should investigate, since it won't work without webpack, I guess.

@@ -67,7 +64,7 @@ export default class FileUpload extends Component {
}
if (nextProps.selectedFile.fileId !== this.state.selectedFile.fileId) {
const file = nextProps.selectedFile;
this.updateFileState(file);
//this.updateFileState(file);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed anymore? Please consider removing

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 don't know who and why made this. This pull requests focus on project structure and build process.

border-collapse: separate;
width: 100%;
float: left;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure float isn't going to cause trouble?

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 don't know about the code of each components. It's part of our next issues when we will decide how to manage common components

Copy link
Contributor

@tpluscode tpluscode Nov 1, 2017

Choose a reason for hiding this comment

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

I mean, this looks like a new style prop here. I worry it could break some UI

});
componentDidMount() {
//Set time out to ensure calculation happens after render
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is setTimeout the best option to synchronise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not. I don't know who and why made this. This pull requests focus on project structure and build process.

@tpluscode tpluscode merged commit 85b3edc into master Nov 2, 2017
@tpluscode tpluscode deleted the task/SCM-762 branch November 2, 2017 11:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants