Skip to content
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

#7108, #7119: Improve Geocarousel section #7130

Merged

Conversation

allyoucanmap
Copy link
Contributor

Description

This PR improve the carousel sections of GeoStory by:

  • refactoring the json structure to centralize the map configuration
  • refactoring the carousel menu to align it to the navigation menu behaviours and avoid compressed cards
  • fix bugs listed here GeoCarousel section for GeoStory #6993 (comment)
  • add icon for this type of section

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix

Issue

What is the current behavior?

#7108
#7119

What is the new behavior?

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • No

Other useful information

@tdipisa tdipisa requested a review from dsuren1 July 21, 2021 11:54
@tdipisa tdipisa added this to the 2021.02.00 milestone Jul 21, 2021
web/client/translations/data.de-DE.json Outdated Show resolved Hide resolved
web/client/translations/data.es-ES.json Outdated Show resolved Hide resolved
web/client/translations/data.fr-FR.json Outdated Show resolved Hide resolved
web/client/translations/data.it-IT.json Outdated Show resolved Hide resolved
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

Hi @allyoucanmap
Changes with respect to draw support and background works fine.
However I did see few change in behavior with respect to carousel card component update which needs to looked into and possibly needs a fix

  • Clicking on carousel card again with map's zoom and position changed, doesn't reset to it's initial position when marker added (is this behavior replaced with map fit feature now ?) cc @tdipisa @allyoucanmap
  • Adding new item, the carousel widget is not scrolled to the added item
    Scrollcontent
  • Scroll event on carousel card item is propagated to other components
    Mousescroll
  • Also carousel item navigation with arrow keys is missing in the new geo carousel widget
  • Infocard position is misaligned due to recent change
    2021-07-22 14_43_30-
  • After deleting an item in carousel, unable to select other items after the deleted item in list
    deleteItem
  • Selecting the card's footer toolbar icons, triggers card selection too (Is this behavior fine? cc @tdipisa)
    autoSelection

@allyoucanmap
Copy link
Contributor Author

Hi @dsuren1,
thanks for the review
done all fixes but these two:

Clicking on carousel card again with map's zoom and position changed, doesn't reset to it's initial position when marker added (is this behavior replaced with map fit feature now ?) cc @tdipisa @allyoucanmap

yes, the new behaviour follow the fit feature workflow

Also carousel item navigation with arrow keys is missing in the new geo carousel widget

The issue was referring to the arrows in the UI so for now we don't need an implementation for keyboard

@allyoucanmap allyoucanmap requested a review from dsuren1 July 22, 2021 13:29
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

  • Selecting the card's footer toolbar icons, triggers card selection too (Is this behavior fine? cc @tdipisa)

Happens with delete icon (Edit doesn't trigger the selection )

  • Popup support needs to be disabled when drawing, as it interferes with the draw displaying popup after placing the marker
  • Small cosmetic, just so we know, in mobile view having two geocarousel section one after another, displays the second carousel widget partially in the bottom
    2021-07-23 15_24_03-MapStore HomePage
  • Infocard position is misaligned due to recent change

Info card style needs to be adjusted

Apart from these, the fixes works fine.

@allyoucanmap
Copy link
Contributor Author

Small cosmetic, just so we know, in mobile view having two geocarousel section one after another, displays the second carousel widget partially in the bottom

@dsuren1 done apart of this. Now the implementation uses position sticky so it's the default behaviour I would discuss before with @tdipisa but I don't think is a blocker. We will open a new issue if needed

@dsuren1
Copy link
Contributor

dsuren1 commented Jul 23, 2021

LGTM :)

@dsuren1 dsuren1 added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jul 23, 2021
@allyoucanmap allyoucanmap merged commit 6677dd5 into geosolutions-it:master Jul 23, 2021
allyoucanmap added a commit to allyoucanmap/MapStore2 that referenced this pull request Jul 23, 2021
@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants