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

Move component props from stash to stash.component_props #626

Merged
merged 4 commits into from
Mar 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/MusicBrainz/Server.pm
Expand Up @@ -440,19 +440,14 @@ sub TO_JSON {
current_language_html
entity
hide_merge_helper
instrument_types
instruments_by_type
isrcs
jsonld_data
last_replication_date
linked_entities
makes_no_changes
merge_link
new_edit_notes
recordings
server_details
server_languages
tag
to_merge
);

Expand Down
5 changes: 4 additions & 1 deletion lib/MusicBrainz/Server/Controller/ISRC.pm
Expand Up @@ -35,9 +35,12 @@ sub show : Chained('load') PathPart('')
my @recordings = sort_by { $_->name } $c->model('Recording')->load(@$isrcs);
$c->model('ArtistCredit')->load(@recordings);
$c->stash(
recordings => \@recordings,
current_view => 'Node',
component_path => 'isrc/Index.js',
component_props => {
isrcs => $isrcs,
Copy link
Member

Choose a reason for hiding this comment

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

Ok, they're already in the stash so normally I would say not to serialize them twice, but the isrcs should be a small list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I remove them from the stash earlier on then?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, perhaps. I thought other actions might be using them since they are set in _load, but it looks like this is the only action in this controller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, which is why I didn't touch it.

recordings => \@recordings
}
);
}

Expand Down
6 changes: 4 additions & 2 deletions lib/MusicBrainz/Server/Controller/Instrument.pm
Expand Up @@ -157,10 +157,12 @@ sub list : Path('/instruments') Args(0) {
}

$c->stash(
instruments_by_type => $entities,
instrument_types => \@types,
current_view => 'Node',
component_path => 'instrument/List',
component_props => {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unsatisfactory to create broken commits like this - if someone bisecting lands on this commit, the page will be broken for them because the component isn't changed to understand this until b137a99 from what I can tell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

instruments_by_type => $entities,
instrument_types => \@types
}
);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/MusicBrainz/Server/Controller/Tag.pm
Expand Up @@ -78,7 +78,7 @@ sub not_found : Private
$c->stash(
current_view => 'Node',
component_path => 'tag/NotFound.js',
tag => $tagname,
component_props => {tag => $tagname}
);
$c->detach;
}
Expand Down
46 changes: 25 additions & 21 deletions root/instrument/List.js
@@ -1,4 +1,5 @@
/* @flow
/*
* @flow
* Copyright (C) 2018 Shamroy Pellew
*
* This file is part of MusicBrainz, the open internet music database,
Expand All @@ -15,59 +16,62 @@ const {l} = require('../static/scripts/common/i18n');
const {lp_attributes} = require('../static/scripts/common/i18n/attributes');
const {l_instrument_descriptions} = require('../static/scripts/common/i18n/instrument_descriptions');

type PropsT = {|
+instrument_types: $ReadOnlyArray<InstrumentTypeT>,
+instruments_by_type: {|
+[number]: $ReadOnlyArray<InstrumentT>,
+unknown: $ReadOnlyArray<InstrumentT>,
|},
|};

const Instrument = ({instrument}) => (
<li>
<EntityLink entity={instrument} />
{instrument.description
? <Frag>
? (
<Frag>
{' — '}
<span
className="description"
dangerouslySetInnerHTML={{
__html: l_instrument_descriptions(instrument.description),
}}
dangerouslySetInnerHTML={{__html: l_instrument_descriptions(instrument.description)}}
/>
</Frag>
)
: null}
</li>
);

const InstrumentList = () => {
const instrumentTypes = $c.stash.instrument_types;
if (!instrumentTypes) {
return null;
}

const instrumentsByType = $c.stash.instruments_by_type;
if (!instrumentsByType) {
return null;
}

const InstrumentList = ({
instrument_types: instrumentTypes,
instruments_by_type: instrumentsByType,
}: PropsT) => {
const unknown = instrumentsByType.unknown;

return (
<Layout title={l('Instrument List')} fullWidth={true}>
<Layout fullWidth title={l('Instrument List')}>
<div id="content">
<h1>{l('Instrument List')}</h1>
{instrumentTypes.map(type => (
<Frag>
<Frag key={type.id}>
<h2>{lp_attributes(type.name, 'instrument_type')}</h2>
<ul>
{instrumentsByType[type.id].map(instrument => (
<Instrument key={instrument.id} instrument={instrument} />
<Instrument instrument={instrument} key={instrument.id} />
))}
</ul>
</Frag>
))}
{(unknown && unknown.length)
? <Frag>
? (
<Frag>
<h2>{l('Unclassified instrument')}</h2>
<ul>
{unknown.map(instrument => (
<Instrument key={instrument.id} instrument={instrument} />
<Instrument instrument={instrument} key={instrument.id} />
))}
</ul>
</Frag>
)
: null}
</div>
</Layout>
Expand Down
14 changes: 6 additions & 8 deletions root/isrc/Index.js
Expand Up @@ -17,15 +17,13 @@ const {l, ln} = require('../static/scripts/common/i18n');
const {artistCreditFromArray} = require('../static/scripts/common/immutable-entities');
const formatTrackLength = require('../static/scripts/common/utility/formatTrackLength');

const Index = () => {
const isrcs = $c.stash.isrcs;
const recordings = $c.stash.recordings;
const userExists = $c.user_exists;

if (!isrcs || !recordings) {
return null;
}
type PropsT = {|
+isrcs: $ReadOnlyArray<IsrcT>,
+recordings: $ReadOnlyArray<RecordingT>,
|};

const Index = ({isrcs, recordings}: PropsT) => {
const userExists = $c.user_exists;
const isrc = isrcs[0];
return (
<Layout fullWidth title={l('ISRC “{isrc}”', {isrc: isrc.isrc})}>
Expand Down
6 changes: 3 additions & 3 deletions root/tag/NotFound.js
Expand Up @@ -11,14 +11,14 @@ const React = require('react');
const NotFound = require('../components/NotFound');
const {l} = require('../static/scripts/common/i18n');

const TagNotFound = () => (
const TagNotFound = ({tag}: {|+tag: string|}) => (
<NotFound title={l('Tag Not Used')}>
<p>
{l('No MusicBrainz entities have yet been tagged with "{tag}".', {tag: $c.stash.tag})}
{l('No MusicBrainz entities have yet been tagged with "{tag}".', {tag: tag})}
</p>
<p>
{l('If you wish to use this tag, please {url|search} for the entity first and apply the tag using the sidebar.',
{__react: true, search_url: '/search'})}
{__react: true, url: '/search'})}
</p>
</NotFound>
);
Expand Down
8 changes: 0 additions & 8 deletions root/types.js
Expand Up @@ -99,14 +99,6 @@ type CatalystUserT = {|
|};

type CatalystStashT = {|
+instruments_by_type?: {|
+[number]: $ReadOnlyArray<InstrumentT>,
+unknown: $ReadOnlyArray<InstrumentT>,
|},
+instrument_types?: $ReadOnlyArray<InstrumentTypeT>,
+isrcs?: $ReadOnlyArray<IsrcT>,
+recordings?: $ReadOnlyArray<RecordingT>,
+tag?: string,
|};

type CommentRoleT = {|+comment: string|};
Expand Down