Skip to content

[Feature] Added possibility to upload multiple images #425

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

MaksymShokin
Copy link
Contributor

@MaksymShokin MaksymShokin commented Aug 25, 2020

@sofiiakvasnevska

Issue

Fliplet/fliplet-studio#4865
https://weboo.atlassian.net/browse/OD-19

Description

Remade PR after reverting and fixed an issue mentioned by Inna.

Screencast

https://streamable.com/sgegno

Backward compatibility

This change is fully backward compatible.

Reviewers

@upplabs-alex-levchenko

@sofiiakvasnevska sofiiakvasnevska self-assigned this Aug 26, 2020
@tonytlwu
Copy link
Contributor

@MaksymShokin
Copy link
Contributor Author

@tonytlwu
Added new styling

@MaksymShokin
Copy link
Contributor Author

mobile
desktop

}

.agenda-multiple-images-item img {
object-fit: contain;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work in IE 11? https://caniuse.com/#feat=object-fit says it doesn't.

flex-wrap: wrap;
align-items: center;
justify-content: center;
width: 149px;
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this making sure that it's always presented as a 2 or 3 column layout in squares if the width and height are set in absolute values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonytlwu
To make sure that the column has 2 or 3 items per row we can add max-width to the images container in px. So, far this is the only way we have found. Is it ok ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaksymShokin I don't understand what the challenge is here. There are many methods in CSS to create a 2 or 3 column layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonytlwu
Columns-count property does not work properly here due to wrong ordering. Could you share other methods you have in mind? I can use css grid. But we still need to set absolute values for height and width.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonytlwu
I do not see any changes. If I add fourth item it does not go onto the next row.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaksymShokin Sorry. The link was incorrect. See https://jsfiddle.net/tonytlwu/7r26b8cz/3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonytlwu
In the requirements, it was said that containers should be squares. You added width: 33.33%. How can we add the same value to the height?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use either one of these principles https://jsfiddle.net/tonytlwu/bL2hkcae/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

widget.json Outdated
"name": "List (from data source)",
"package": "com.fliplet.dynamic-lists",
"name": "List (from data source)-images",
"package": "com.fliplet.dynamic-lists-images",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"package": "com.fliplet.dynamic-lists-images",
"package": "com.fliplet.dynamic-lists",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


agendaImageGallery.listen('afterChange', function() {
Fliplet.Page.Context.update({
agendaImageGalleryId: _this.data.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we updating these page context parameters? Does this feature support opening a detail view gallery item using URL parameters? If so, I don't see any code that supports this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is how it is done in the gallery component. I do not think that is it possible to open a detail view using URL, as far as I remember this is done to correctly assign images to specific component. So if we have multiple gallery components it will not show all pictures from both of those components.
images

Copy link
Contributor

Choose a reason for hiding this comment

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

If the page context updating isn't going to do anything for the user, then take it out. It wasn't part of the requirement to get that working.


agendaImageGallery.listen('afterChange', function() {
Fliplet.Page.Context.update({
agendaImageGalleryId: _this.data.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

If the page context updating isn't going to do anything for the user, then take it out. It wasn't part of the requirement to get that working.

@MaksymShokin
Copy link
Contributor Author

@tonytlwu
If I remove it then fullscreen mode does not work for images.

@tonytlwu
Copy link
Contributor

@MaksymShokin Can you explain or show what fullscreen mode you're referring to, and why it doesn't work without the page context?

@MaksymShokin
Copy link
Contributor Author

@tonytlwu
By fullscreen mode I mean this.
fullscreen

@MaksymShokin
Copy link
Contributor Author

@tonytlwu
I do not know why it does not work without it, I copied it from the gallery. Actually you wrote this code in gallery, so I think there is should be logic behind it.
gallery

@tonytlwu
Copy link
Contributor

@MaksymShokin You need to find out why it doesn't work without the page context. As far as I know they shouldn't be necessary to the working of the feature.

By setting the page context, the URL query parameter, e.g. ?agendaImageGalleryId=3, gets updated. These are present only to make sure that when a page loads, it knows to process the query and take users to a specific state of the page.

Without it, gallery feature should work just fine.

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.

3 participants