Skip to content

Conversation

@JadeDickinson
Copy link
Contributor

@JadeDickinson JadeDickinson commented Oct 18, 2020

Why

Shows contributor's name on the List and Tile views of the Contributions page (already shown on Map view).

#537

Pre-Merge Checklist

  • All new features have been described in the pull request
  • Security & accessibility have been considered
  • All outstanding questions and concerns have been resolved
  • Any next steps that seem like good ideas have been created as issues for future discussion & implementation
  • High quality tests have been added, or an explanation has been given why the features cannot be tested
  • New features have been documented, and the code is understandable and well commented
  • Entry added to CHANGELOG.md if appropriate

What

  • Displays Contributor name on Contributions list view
  • Displays Contributor name on Contributions tile view

Screenshot 2020-10-18 at 10 05 38

Screenshot 2020-10-18 at 10 12 38

How

Contributor's name is stored as (Name) along with details of the offer in the name field. I've used a regex to match for text within brackets.

Outstanding Questions, Concerns and Other Notes

I wasn't totally clear on the requirements - do we want the ability to Toggle showing contributor names as well?

If so, how would we want this to look on the page - would a toggle box belong alongside the Filters given that it's not a Filter but a display option?

@h-m-m
Copy link
Collaborator

h-m-m commented Oct 19, 2020

Hi, @JadeDickinson, and thanks for contributing! I'm so happy you're interested in some of this project's open issues

To answer your question, I don't think we'd want a toggle on the filter page itself. We may want a system-wide toggle, or a permissions-based toggle where you only see the name if you have the permission to do so.

That permissions- or settings-based toggle, if and when we add it, would likely live in the Rails side. For that reason, maybe it's better if we split the responsibilities up. For example, we could have the Rails side either supply a name or not (or leave it blank?) and then the Vue components could display the name only when it exists. What do you think about this approach?

@maebeale
Copy link
Collaborator

maebeale commented Jan 8, 2021

@JadeDickinson hello there! we'd love to move forward on this feature. might you have time in the near future to pick this back up, or, might you be ok with our taking it from here?

@JadeDickinson
Copy link
Contributor Author

@maebeale Hi there! I wasn't sure what the favoured approach was at the time which is why I didn't work further on it. I'm not sure I have the time to at the moment, so feel free to take it from here

@maebeale
Copy link
Collaborator

maebeale commented Jan 8, 2021

Thank you for the quick reply, @JadeDickinson ! If/when you're able to hop back in, we'd love your contributions -- which include questions and threads like the above. Best to you!

<td>
<a :href="contribution.respond_path" class="button icon-list is-primary"><span class=""> Respond</span></a>
</td>
<td>{{ contribution.name.match(/\((.*)\)/)[1] }}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

/\((.*)\)/ this regex is identical with the one below. I think you should use some constant to config it somewhere, then use it for both places.

Next time when we want to change this regex, we just need to do it once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea - however I didn't end up merging this PR (see the discussion of the feature above). So I won't be refactoring further for now as this code isn't in main.

@svileshina has taken over work on this feature on #917

@solebared solebared merged commit 161aefd into rubyforgood:main Jun 18, 2021
@svileshina
Copy link
Collaborator

hi @JadeDickinson, we merged this alongside #917. Thank you for your work on this!

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.

6 participants