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

[FEATURE] Filter pins on map to match filters/search for storied sites #4459

Conversation

manav2modi
Copy link
Contributor

Description

Please provide a summary of the pull request and the issue it fixes. Please add necessary details, context, dependencies, explanation of when review is needed (see next section), etc.

Fixes #4458

Review Time Estimate

Please give your idea of how soon this pull request needs to be reviewed by selecting one of the options below. This can be based on the criticality of the issue at hand and/or other relevant factors.

  • Immediately
  • Within a week
  • When possible

Type of changes

Please select a relevant option:

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Other (any another change that does not fall in one of the above categories.)

Checklist:

Please select all applicable options:

  • I have signed the Rokwire Contributor License Agreement (CLA). (Any contributor who is not an employee of the University of Illinois whose official duties include contributing to the Rokwire software, or who is not paid by the Rokwire project, needs to sign the CLA before their contribution can be accepted.)
  • I have updated the CHANGELOG.
  • I have read the Contributor Guidelines.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My change requires updating the documentation.
  • I have made necessary changes to the documentation.
  • I have added tests related to my changes.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@manav2modi manav2modi changed the base branch from develop to support/v6.0 November 5, 2024 18:23
@manav2modi
Copy link
Contributor Author

Thanks @mihail-varbanov the commit you made helped with the functionality. I also added a small fix for the filters and a fix to hide _buildMapExploreBar

@manav2modi
Copy link
Contributor Author

@mihail-varbanov I have also added the feature so that when a user clicks on a pin the color changes so it is obvious what pin is selected. Thanks for the review.

Copy link
Collaborator

@mihail-varbanov mihail-varbanov left a comment

Choose a reason for hiding this comment

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

@manav2modi, I think there are major problems in your last commit Add color pin feature.

  1. The purpose of _selectedMapExplore is to drive the content of MapExploreBar. You should not try to handle the specific Stored Sites features in map explore selection mechanism, they should be handled separately and independently. In short, you should leave the existing selection management in ExploreMapPanel untouched, or only disable it in case of Stored Sites map type selection.

  2. I would like to see the design of what you are doing. For me the current map management for stored sites is confusing, inconsistent and not well thought. I do not know if this is your own design decisions, or this behavior was requested explicitly by the University.

@mihail-varbanov
Copy link
Collaborator

@manav2modi, in addition to above:

  1. My belief is that what you did in this branch is conceptually wrong. If your remember, when you started this feature I told you that filtering on stored sites should be performed in ExploreMapPanel, not in ExploreStoriedSightsBottomSheet, and the filtered places content should be passed to ExploreStoriedSightsBottomSheet for displaying. I also suggested selecting filters to get transferred from ExploreStoriedSightsBottomSheet to ExploreMapPanel via Storage service, but this is not the only possible way, of course. The answer that I got from you (as long as remember) was something like that the filtering should be performed only in ExploreStoriedSightsBottomSheet locally, and the filtered places content in ExploreStoriedSightsBottomSheet would not affect ExploreMapPanel display and selection in any way. What happens in this branch is that ExploreStoriedSightsBottomSheet attempts to control the content and selection in ExploreMapPanel, that is wrong. The direction should be the opposite: ExploreMapPanel should maintain its content and selection, ExploreStoriedSightsBottomSheet can only notify ExploreMapPanel about user activity that requires updating that content or selection.

@manav2modi
Copy link
Contributor Author

@mihail-varbanov Thanks for the review. I have tried to separate out the functionality as to not mess with the explore map features as much and created a new variable to keep track of the selected pin. Hopefully this makes it cleaner and less likely to cause any problems. I have also gone ahead and added storiedSites to the analytics list as well. Thanks!

Copy link
Collaborator

@mihail-varbanov mihail-varbanov left a comment

Choose a reason for hiding this comment

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

The only reason I'm approving this pull request is management's strong desire to release a 6.0 hotfix. I continue to maintain that there are conceptual issues here that should be worked out in calmer times.

@mihail-varbanov mihail-varbanov merged commit e3459bf into support/v6.0 Nov 7, 2024
@mihail-varbanov mihail-varbanov deleted the 4458-feature-filter-pins-on-map-to-match-filterssearch-for-storied-sites branch November 7, 2024 08:48
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.

[FEATURE] Filter pins on map to match filters/search for storied sites
2 participants