Skip to content

Conversation

pamfilos
Copy link
Collaborator

No description provided.

@pamfilos pamfilos changed the title ui: file upload, fileTree, forms: RepoField, TagsInput WIP ui: file upload, fileTree, forms: RepoField, TagsInput Jan 27, 2020
@coveralls
Copy link

coveralls commented Jan 27, 2020

Coverage Status

Coverage remained the same at 69.517% when pulling 64fcf33 on pamfilos:ui-files-async into 5d460cb on cernanalysispreservation:master.

@pamfilos pamfilos changed the title WIP ui: file upload, fileTree, forms: RepoField, TagsInput ui: file upload, fileTree, forms: RepoField, TagsInput Feb 3, 2020
@papadopan
Copy link
Contributor

papadopan commented Feb 4, 2020

Lint errors

  • unused imports

  • PropTypes Validation

  • When you upload an image, then you have to cancel in order to upload another one. This is kind of confusing and misleading.

Screenshot 2020-02-04 at 11 11 05

  • Large names can destroy the ui

Screenshot 2020-02-04 at 11 13 02

  • If many files are uploaded then the ui for the repos upload is not visible

Screenshot 2020-02-04 at 11 14 27

  • Validate again the error messages, when a PR is uploaded then the message is not clear

Screenshot 2020-02-04 at 11 16 23

  • Maybe for consistency reasons, the header of the input should be on top

Screenshot 2020-02-04 at 11 18 53

  • Maybe we should not allow to upload multiple files, or implement a ui to support it

Screenshot 2020-02-04 at 11 20 33

<FileManager key="filesManager" files={this.props.files} />,
// <FileList key="filesList" files={this.props.files} />,
<Box key="filesList" pad="small">
<Box key="filesList" style={{ paddingLeft: "3px", paddingRight: "10px" }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something like this instead
pad={{left:"small", right:"medium"}} in order to keep the grommet values

render() {
return <TreeNode data={this.state.data} />;
return (
<Box style={{ marginLeft: "5px" }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we instead margin={{left:'small'}}

Copy link
Contributor

@papadopan papadopan left a comment

Choose a reason for hiding this comment

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

Check if the suggested updates are required

Signed-off-by: Pamfilos Fokianos <pamfilosf@gmail.com>
@annatrz annatrz merged commit ca95738 into cernanalysispreservation:master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants