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

Add user filter for beatmap discussions #3090

Merged
merged 46 commits into from May 8, 2018

Conversation

4 participants
@notbakaneko
Contributor

notbakaneko commented Apr 26, 2018

fixes #2105

Entirely client-side.

"This will be a quick change"; and then it wasn't

a
children: children
className: classNames
href: '#'

This comment has been minimized.

@notbakaneko

notbakaneko Apr 26, 2018

Contributor

need something else for this

@peppy peppy assigned MillhioreF and nanaya and unassigned MillhioreF May 7, 2018

@@ -183,7 +183,7 @@ class BeatmapDiscussions.Discussions extends React.PureComponent
@props.mode == 'timeline' && @currentSort() == 'timeline'
sortedDisussions: ->
sortedDiscussions: (discussions) ->

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

parameter not used?

@@ -213,6 +219,7 @@ class @BeatmapDiscussionHelper
# empty path segments are ''
mode: if _.includes(@MODES, mode) then mode else @DEFAULT_MODE
filter: if _.includes(@FILTERS, filter) then filter else @DEFAULT_FILTER
user: parseInt(url.searchParams.get('user'), 10) if url.searchParams.get('user')?

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

apparently there's already params variable assigned with url.searchParams (which isn't used =D )

This comment has been minimized.

@notbakaneko

notbakaneko May 8, 2018

Contributor

and into the bin it goes

@@ -58,6 +59,11 @@ class BeatmapDiscussions.Main extends React.PureComponent
$.subscribe 'beatmapsetDiscussions:update.beatmapDiscussions', @update
$.subscribe 'beatmapDiscussion:jump.beatmapDiscussions', @jumpTo
$.subscribe 'beatmapDiscussionPost:markRead.beatmapDiscussions', @markPostRead
$.subscribe 'beatmapsetDiscussions:userFilterChanged.beatmapDiscussions', (_e, { selectedUserId }) =>

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

or update update function to handle user parameter? (also cache reset is already done in componentWillUpdate)

This comment has been minimized.

@notbakaneko

notbakaneko May 8, 2018

Contributor

update has state races which allows things like this to happen
image

This comment has been minimized.

@notbakaneko

notbakaneko May 8, 2018

Contributor

might be browser just failing too

@@ -199,7 +207,7 @@ class BeatmapDiscussions.Main extends React.PureComponent
modes[mode] = {}
for d in @state.beatmapset.discussions
for d in @filterDiscussions(@state.beatmapset.discussions)

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

this confuses nominate button (wrong unresolved count).

@@ -179,6 +186,7 @@ class BeatmapDiscussions.Main extends React.PureComponent
countsByBeatmap = {}
countsByPlaymode = {}
totalHype = _.filter(@state.beatmapset.discussions, message_type: 'hype', deleted_at: null).length

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

this can be part of the big loop...?

* along with osu!web. If not, see <http://www.gnu.org/licenses/>.
*/
.beatmap-discussions-user-filter {

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

can't this be a modifier of beatmap-list? (maybe with class name change)

This comment has been minimized.

@notbakaneko

notbakaneko May 8, 2018

Contributor

more like move to a more generic select-options component and use fragments to render

...which comes later

return if event.button != 0
event.preventDefault()
selectedUserId = user.id

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

pass key: value directly in the $.publish call? ಠ_ಠ

a
children: children
className: classNames
href: "?user=#{key ? ''}"

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

use the helper .url instead?

&--stats {
flex: none;
display: flex;
flex-direction: column;

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

something here seems to cause incorrect alignment

@@ -56,6 +56,30 @@
}
}
&__filter-group {
min-width: 0;
flex: 1 1 auto;

This comment has been minimized.

@nanaya

nanaya May 7, 2018

Collaborator

is the full width for beatmap dropdown intentional?

This comment has been minimized.

@notbakaneko

notbakaneko May 8, 2018

Contributor

yes

a
children: children
className: classNames
href: BeatmapDiscussionHelper.url user: key

This comment has been minimized.

@nanaya

nanaya May 8, 2018

Collaborator

may want to update the function to allow updating instead of rebuilding from scratch =] (probably should've done this way earlier)

This seems to work

{...} =
    if update ? false then _.assign(@urlParse(), options) else options

@nanaya

nanaya approved these changes May 8, 2018

@nanaya nanaya merged commit e94755d into ppy:master May 8, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notbakaneko notbakaneko deleted the notbakaneko:feature/beatmap-discussions-user-filter branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment