Simplify the code rendering the map #5108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
app/assets/javascripts/map.js
Outdated
| }); | ||
| }, | ||
| getPopupContent: function(data) { | ||
| return "<a href='/budgets/" + data.budget_id + "/investments/" + data.investment_id + "'>" + data.investment_title + "</a>"; |
There was a problem hiding this comment.
Line 213 exceeds the maximum line length of 110.
javierm
force-pushed
the
map_refactoring
branch
3 times, most recently
from
2089f8f
to
7118e4e
Compare
javierm
force-pushed
the
map_refactoring
branch
from
7118e4e
to
9846993
Compare
javierm
force-pushed
the
map_refactoring
branch
3 times, most recently
from
1907495
to
1c94694
Compare
The only view that linked to this action was never used and so it was deleted in commit 0bacd5b. Since now the proposals controller is the only one place rendering the `shared/map` partial, we're moving it to the proposals views.
We're calling this method after setting the map location with `map_location = MapLocation.new if map_location.nil?`, so the condition `map_location.present?` is always going to be true.
This way it'll be easier to refactor it.
Since we aren't using helpers anymore, we don't need the `map_location` prefix.
The calls to `render_map` are confusing since there are so many parameters. We can assume that the map is editable if we pass the remove marker label.
We were passing `nil` in some calls, which was confusing. Since now we've got two optional parameters, we're using named parameters.
We were probably setting them separately to avoid having blank data attributes in the HTML. However, when a data attribute is `nil`, Rails doesn't write it in the HTML in the first place.
We were manually generating the IDs in order to pass them as data attributes in the HTML in a component where we don't have access to the form which has the inputs. However, these data attributes only make sense when there's a form present, so we can pass the form as a parameter and use it to get the IDs. We can now define a map as editable when there's an associated form, which makes sense IMHO.
Rails forms automatically take the value from the object related to the form.
We had two different keys with the same text and were passing it as a parameter. Since the text is the same in any case, we don't need a parameter for it. Note we are using the `proposals` i18n key instead of creating a new one in a `shared` namespace one because creating a new key would mean that we'd lose the already existing translations in Crowdin.
We had 130 lines long function, and we're trying to reduce its size so it's easier to follow the code.
The `marker` variable is like a global variable inside the `initializeMap` function, so assigning it inside the `createMarker` function was changing its value in other places. So we're using different variable names like `newMarker` in order to make the code easier to follow. Now we "only" change the `marker` variable in functions that modify the marker.
taitus
approved these changes
May 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives