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

Private topic form user names #183 #199

Merged
merged 1 commit into from Apr 6, 2016
Merged

Private topic form user names #183 #199

merged 1 commit into from Apr 6, 2016

Conversation

glebm
Copy link
Collaborator

@glebm glebm commented Apr 6, 2016

new-private-topic

  • Autocomplete user names remotely. Private topic form user names #183
  • Fix autocomplete styles to use thredded form styles.
  • Add $thredded-form-color/background variables.
  • Avoid creating notification jobs when seeding, bump the number of
    records, and log progress.
  • Fix includes in topics and private topics controller. There is still
    an issue of loading read state for each topic, but this can't be fixed
    easily due to the decorator architecture.
  • Update DbTextSearch to 0.2.0 for indexed prefix matching in user search.
  • Fix Messageboard slugs to allow creating messageboards with reserved
    names.
  • Update changelog

@glebm glebm force-pushed the private-topics-users branch 3 times, most recently from 31ad347 to abc44cc Compare April 6, 2016 19:02
friendly_id :slug_candidates,
use: [:slugged, :reserved],
# Avoid route conflicts
reserved_words: %w(messageboards private-topics autocomplete-users theme-preview)
Copy link
Member

Choose a reason for hiding this comment

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

I like how you deferred slug_candidates to the private method below, returning that array. What about doing the same with this array of possible route conflicts? That removes the need for the comment to describe the "why". The method name would describe it sufficiently

# ...
reserved_words: possible_route_conflicts

private

def possible_route_conflicts
  %w(messageboards private-topics autocomplete-users theme-preview)
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Acknowledged, but can't pass a symbol here, and any method called here would need to be defined beforehand (above). We could add a constant but then it can't be private I think.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, man. Good point. Forgot that that is in the class scope.

If there's a way to accomplish that where it's not obnoxious, then I'd lean towards that but if not - don't worry about it.

@jayroh
Copy link
Member

jayroh commented Apr 6, 2016

Just a few comments on this but I'll leave them up to you :)

Otherwise, let's get this merged in when you're ready!

* Autocomplete user names remotely.
* Fix autocomplete styles to use thredded form styles.
* Add `$thredded-form-color/background` variables.
* Avoid creating notification jobs when seeding, bump the number of
  records, and log progress.
* Fix `includes` in topics and private topics controller. There is still
  an issue of loading read state for each topic, but this can't be fixed
  easily due to the decorator architecture.
* Update DbTextSearch to 0.2.0 for indexed prefix matching in user search.
* Fix Messageboard slugs to allow creating messageboards with reserved
  names.
* Update changelog
* Readme: advise to add username index.
@glebm glebm merged commit b382ef5 into master Apr 6, 2016
@glebm glebm deleted the private-topics-users branch April 6, 2016 20:20
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.

None yet

2 participants