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

Search: Fixes the spacing issue between suggested artworks (AIC-633) #352

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

sam33rdhakal
Copy link
Contributor

The cause behind this issue was the logic we used to get column number for suggested artworks. That logic was based on the position of the on the map cell.

val columnValue = (currentPosition - onTheMapPosition) % maxSize

This logic can go wrong when suggested artwork cells are recycled before that on the map cell. So the proposed solution is to not use on the map cell at all. Now I pass the order of the cells when they are constructed and then use it to calculate the column index.

val columnValue = viewModel.order % maxSize

Copy link
Contributor

@Cliabhach Cliabhach left a comment

Choose a reason for hiding this comment

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

One comment; everything else looks great.

@@ -85,7 +85,8 @@ class SearchTourCellViewModel(val articTour: ArticTour) : SearchBaseListItemView
/**
* ViewModel for displaying the circular artwork image under "On the map" section.
*/
class SearchCircularCellViewModel(val artwork: ArticObject?) : SearchBaseCellViewModel() {
class SearchCircularCellViewModel(val artwork: ArticObject?,
override val order: Int = -1) : SearchBaseCellViewModel(), OrderedCellViewModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the default value for order here. That way the compiler will error out if we forget to include a number.

Copy link
Contributor

@Cliabhach Cliabhach left a comment

Choose a reason for hiding this comment

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

Great! Let's get this in.

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