Nav link styles, 'Where am I' as text, legend improvements

[eloisejones]

Description

Addresses #42 #29 #53 , including:

• Nav Links now will not be affected by "visited" status.
• 'Where am I' text is now text instead of CSS. (future improvement: make it function with the rest of the button)
• Improved legend functionality for small screens; wraps up and to the right for large screens
• Added geocode portion of map control into standard top-left map control area to standardize its implementation.

List of changes

• 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)

Checklist:

• This pull request relates to a ticket.
• This code is tested.
• I want my code added to the response theme.

[omnilord]

Firstly, let me say welcome and thank you for your interest in contributing. It is great to see enthusiasm for the project and a willingness to contribute to something that could benefit thousands of people during disaster events.

That being said, this PR has critical issues. I think the root cause stems from some communication short falls with existing contributors about how the software was designed prior to diving in and making sweeping, breaking changes. I understand you checked briefly with @nihonjinrxs, but he understood the change as style modifications, rather than the large refactor included in this PR. He would also have directed you to discuss this thoroughly with the team before making these changes had he known your intent.

I am attempting to be constructive (please forgive me if I come across as blunt!), but I'm just going to dive in. Some of the things I noticed when trying to review this PR:

1. There are unresolved merge segments in the source which means this PR has not been tested. I realize we don't have an automated test suite for this particular repository (I'm not sure how we could even set that up from a test structure perspective) but this means more rigor should be made to manually review changes before making a pull request. The repository is early in it's development, early enough that a CONTRIBUTING.md is still being worked out, and unfortunately, that has led to ambiguity in how to participate. This is a major take away for the whole team and will be addressed soon.

2. This pull request seems to update for multiple, unrelated issues which goes against the generally-accepted rule of small, single issue pull requests where possible (both @nihonjinrxs and I have dinged each other guilty of sweeping up multiple issues in the past, so you're not alone in that respect ;) ). Smaller PRs are easier to review, easier to test, easier to spot bad practices, and most importantly easier to correct. Looking at your other PRs (Map refactoring, click-zoom for circle clusters, and improved config org/doc #50, 'Where am I' verbage change, cluster opacity #51, and Refactoring/Reorganizing HTML and CSS Components #54), it appears you have committed everything against your local master and only branched for pull requests after the fact. Most projects try to adhere to a Git Flow as the general rule (we've been talking about using rebase in lieu of merges where appropriate). If you are unfamiliar with Git Flow, I recommend reading https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow but there are plenty of other resources available on best practices with git. As of right now, the volume of changes here mixed together introduce features, fixes, and a massive refactor all in one. I cannot tell where the update for "Where am I" was made; it's lost in the large body of refactoring. Which brings me to...

3. There are unusually large changes to the basic file structure that indicate a lack of research and inquiry with existing team members into how the current build was reached. This repository represents a theme gem not a final production site. The modifications in this PR changes how the final deployment functions and disregards the build process already in place. Given that this PR is untested (see first point), this is a huge red flag. It’s worth noting that some of these changes (the inclusion of theme gems in the Gemfile and config files) would actually break all the sites dependent on the theme.

4. The conversion to SASS was not necessary and showed particular disregard for prior work, in view of the fact that @miklb had mentioned his in-progress work to make the build process easier using PostCSS and Gulp in the slack earlier. (https://cfa.slack.com/archives/CCQKTUQKT/p1563023792060800).

This project has tremendous potential to do good but we have to have an open responsive dialog about what changes are being made so we don't misunderstand how the project is structured or commit changes that break functionality, especially during a response effort. This ensures that no one ends up step on each others' toes with conflicting changes for different issues. That's one of the reasons why we request joining the slack channel as one of the first things in the "Getting Involved" document.

I hope this review has not discouraged you from participating. We look forward to working with you on this effort in the future.

[eloisejones]

Thank you for taking the time to review and reply. I definitely did not 'check in briefly' with only one person on this project. I was in regular communication with multiple contributors on slack. I was given to understand that:

• the UI was in major need of addressing
• that this was a newer project in need of substantial work to facilitate the ideal smaller, simple PR, improve the deployment process for new relief events, and allow newer contributors easy-wins
• that I should really just dig in and make it happen

1. That is quite an assumption and accusation to level at someone. I would never submit untested code. It was tested thoroughly on my local machine. It was understood among other contributors that because of the PR backlog and the structural changes I was making, we would likely need to put in some manual effort for the final merge. There are no unresolved merge segments locally.
2. I understand the smaller PR model. Again, I was given to understand the project needed sweeping change (see above) and that contributors were interested in working to discuss and incorporate larger changes once the current relief effort wrapped. I did neglect to create a branch for my first change (distraction), but all others definitely are committed against a branch. I am familiar with GitFlow.
3. Sweeping change called for substantial change to the file structure - versioning the theme would allow the existing sites to continue working and move forward with new changes. I was given to understand that the project was early enough that there was the ability to change deployment to facilitate important changes in the theme. Team members were busy working on the current deployment, and I received conflicting instructions to both stop work and continue work on the interface. My inclusion of the additional gem was me attempting to review and include PRs being made to the deployed model.
4. There were duplicate style definitions in multiple places, and it was noted by more than one contributor that changes to _inc files simply did nothing, even when gulping/rebuilding; only changes to asset/css/main not the (I don't mean the _site version) were having any impact and that was a single enormous style sheet. I was attempting to fix/leverage what had been done with the separate sheets, as it more easily facilitates multiple contributors and is simpler to maintain than one large file.

Yes, open and responsive dialog is critical. It was made clear multiple times that the primary contributors were busy with the current response and were not open to discussing anything other than that. But I was also told I should continue my work and we would discuss later. I have no problem working in ambiguity, so I moved forward.

I assumed discussing it later meant discussion, not: being publicly accused of not testing or researching my work; dismissal of substantial effort I put in to help an early project be more receptive to future contributions and improve its UI and abstraction to enable future enhancements; insinuations of my being unwilling to communicate, when that was the very opposite of what happened. I assumed that those reviewing would actually take the time to understand what I'd done.

The situation is now clearer. I will redirect my efforts elsewhere.