Skip to content

[WIP][DO NOT MERGE]Album feature#339

Open
Kailash0311 wants to merge 2 commits into
crux-bphc:masterfrom
Kailash0311:album_feature
Open

[WIP][DO NOT MERGE]Album feature#339
Kailash0311 wants to merge 2 commits into
crux-bphc:masterfrom
Kailash0311:album_feature

Conversation

@Kailash0311

Copy link
Copy Markdown
Collaborator

No description provided.

@Kailash0311

Copy link
Copy Markdown
Collaborator Author

And rebasing entire atmos-18 branch.

@Kailash0311 Kailash0311 changed the title Album feature [WIP][DO NOT MERGE]Album feature Oct 23, 2018
@aero31aero

Copy link
Copy Markdown
Member

We talked about not rebasing the entire branch and I thought we agreed that this issue should be resolved on master and just cherry-picked on atmos-2018.

@Kailash0311

Copy link
Copy Markdown
Collaborator Author

I think we can cherry pick later. Right now rebased to be consistent with pybits commits. We can later remove the sponsors and fob commits.

Comment thread forms/add-images.js
placeholder: "E.g: FOB,SPONS",
editable: true,
type: "select",
options: ["FOB", "SPONSORS"],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You sure about using a "select" here? Also, I suggest we keep such identifier names consistent by enforcing lowercase and hyphen separated strings.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. They don't have any other option , neither can they write it on their own.

Comment thread forms/add-images.js
label: "Title",
name:"title",
editable: true,
type: "textarea",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Titles probably shouldn't be multiline.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Cons of making in multi-line ?

Comment thread forms/add-images.js
typeahead: false,
none: true
});
module.exports = fields; No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Newline thingy.

var model = mongoose.model('images', imageSchema);
var images = []; // stores image names as string.


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Too many blank lines?

Comment thread routes/api/services/images.js Outdated
var imagePath = (req.body.image).trim();

fs.readdir("public/static/data/images", function(err, files) {
files.forEach(file => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not too keen about this logic here. The image upload mixin provides the path to the image just uploaded as well, and that should then be sent by the concerned form.

.done(function (response) {
	$(editor).find('.status').addClass("success");
	$(editor).attr('value', response.path);
}

See static/scripts/portal.js function submit_item() for how to use the uploaded image name.

text: 'Images',
route: '/images',
};
if (superuser)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Braces around the if block.

script(type="text/javascript" src="/static/lib/jquery.uploadPreview.min.js")
.immersive-wrapper
#add-images-album-wrapper
- var form = { fields: fields, method: "PUT", action: "/api/users/me"};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Accidental remnant of copy-paste in the action field. Put as blank or remove it completely, I suppose, since AJAX is handling it.

@@ -0,0 +1,10 @@
include ../mixins/form
script(type="text/javascript" src="/static/lib/jquery.uploadPreview.min.js")
.immersive-wrapper

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure about using immersive-wrapper here? Use generic div instead, as immersive CSS might sometime later cause this particular page to look out of place from the rest of the portals.

#submit-button
button Submit
script(src="/static/scripts/add-images.js")

No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trailing whitespace.

div(class="cropit-preview", id="cropit-preview-#{data.id}")
div.status
i.icon-check.response(id="export-img-#{data.id}")
i.icon-check.response(id="export-img-#{data.id}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Trailing whitespace.

@aero31aero aero31aero left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a lot of minor comments, and one major one. Looks great so far, the general direction is awesome.

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.

2 participants