Skip to content

BBD-451: Update URL beautifiers to support full URL spec#199

Open
Guo-Haowei wants to merge 34 commits intodevelopfrom
BBD-451
Open

BBD-451: Update URL beautifiers to support full URL spec#199
Guo-Haowei wants to merge 34 commits intodevelopfrom
BBD-451

Conversation

@Guo-Haowei
Copy link

Please review.

Haowei-Guo and others added 30 commits May 29, 2017 09:50
Haowei-Guo added 2 commits June 6, 2017 10:48
to remove prefix without reconstructing and reparsing url
}
} else {
if (paths.length < 2) {
throw new Error('path has wrong number of parts');
Copy link

Choose a reason for hiding this comment

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

I would say "path has too few parts" or "path has fewer than two parts"

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it needs to be changed because the paths in this context excluded productId and productTiltle, so the actual path length is longer than 2.

});
}
}
result['refinements'] = refinements;
Copy link

Choose a reason for hiding this comment

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

Use result.refinements and add a blank line before.

Copy link
Author

Choose a reason for hiding this comment

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

I've already changed it to result.refinements.

Copy link
Contributor

@effervescentia effervescentia left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of small fixes here and there

return `/${paths.map((path) => encodeURIComponent(path.replace(/\s/g, '-'))).join('/')}`;
}

private refinementsComparator(refinement1: SelectedValueRefinement, refinement2: SelectedValueRefinement): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

can be static

}, {});

detail.refinements.sort(this.refinementsComparator).forEach((refinement) => {
if (!(refinement.navigationName in refinementsToKeys))
Copy link
Contributor

Choose a reason for hiding this comment

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

brackets

}
}

paths.push(detail.productID);
Copy link
Contributor

Choose a reason for hiding this comment

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

support for query parameter here

import { Beautifier, BeautifierConfig, Detail } from './interfaces';
import { SelectedValueRefinement } from 'groupby-api';

export class DetailUrlGenerator {
Copy link
Contributor

Choose a reason for hiding this comment

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

make an interface

}
}

export class DetailUrlParser {
Copy link
Contributor

Choose a reason for hiding this comment

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

make an interface


while (path.length > 0) {
const value = this.decode(path.shift());
const navigationName = path.shift();
Copy link
Contributor

Choose a reason for hiding this comment

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

only decode the one?

}

private extractRefinements(refinementString: string, navigationName: string): SelectedValueRefinement[] {
const refinementStrings = refinementString.split('~');
Copy link
Contributor

Choose a reason for hiding this comment

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

no tildes, no

navigation: '/navigation'
}
};
private queryGenerator: QueryUrlGenerator = new QueryUrlGenerator(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use factory pattern here

}
keys.push(key);
}
if (this.config.queryToken.length !== 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant

}

build(query: Query) {
return this.queryGenerator.build(query);
Copy link
Contributor

Choose a reason for hiding this comment

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

build() already exists (buildQueryUrl())

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.

3 participants