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.

Sign up for GitHub

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

Jump to bottom

feat(store): add several Australian stores, add some 3080 brands/models #1367

Merged
merged 15 commits into from
Dec 13, 2020
Merged

feat(store): add several Australian stores, add some 3080 brands/models #1367

merged 15 commits into from
Dec 13, 2020

Conversation

kin3tik
Copy link
Contributor

@kin3tik kin3tik commented Dec 12, 2020

Description

Added several Australian stores:

  • BPCTech (AU)
  • Centrecom (AU)
  • CPL (AU)
  • Mwave (AU)
  • PCCG (AU)
  • Scorptec (AU)
  • Umart (AU)

Added some missing RTX 3080 brands/models:

  • colorful (igame advanced oc, igame vulcan oc)
  • galax (sg, sg oc)
  • gigabyte (aorus xtreme waterforce wb)
  • leadtek (hurricane)

@kin3tik kin3tik requested a review from jef as a code owner December 12, 2020 13:44
jef
jef reviewed Dec 12, 2020
View reviewed changes
Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Oh wow... Quite a lot of stores added! That's awesome. Thank you so much for your time spent on these!

It looks like there is just a small linting issue. Easiest way to fix that is to run npm run lint:fix.

Once this passes linting, we'll get this merged in.

Thank you very much!

@kin3tik
Copy link
Contributor Author

kin3tik commented Dec 12, 2020

Looks like a recent commit broke the stores. I'll add the currency prop.

jef
jef reviewed Dec 12, 2020
View reviewed changes
Copy link
Owner

@jef jef left a comment •
edited

Choose a reason for hiding this comment

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

OK! We're just about there. I think there could be some potential issues with the text in the labels, only because we don't do any trimming on the text.

We should, but not sure of the side effects.

Let me know what you think. I appreciate all the support!

src/store/model/scorptec.ts Outdated Show resolved Hide resolved
src/store/model/umart.ts Outdated Show resolved Hide resolved
src/store/model/bpctech.ts Outdated Show resolved Hide resolved
@kin3tik @jef
Apply suggestions from code review
cb33be7 
Co-authored-by: Jef LeCompte <jeffreylec@gmail.com>
@kin3tik
Copy link
Contributor Author

kin3tik commented Dec 12, 2020

Sounds fair. I wasn't sure how the container text was treated (is it just a contains?) so I was just using the result of innerText on the query selector verbatim.

@kin3tik kin3tik requested a review from jef December 12, 2020 15:08
@jef
Copy link
Owner

jef commented Dec 13, 2020

Sounds fair. I wasn't sure how the container text was treated (is it just a contains?) so I was just using the result of innerText on the query selector verbatim.

Yup! It should mostly work. I've sure it would've worked for the most part too. I have updated mostly just for consistency purpose now.

Thanks for working on this, it's a great addition! I'm sure there are some Aussie folks out there that can appreciate this!

@jef jef changed the title feat(store): added several Australian stores (AU), added a few mission RTX 3080 brands/models feat(store): add several Australian stores, add some 3080 brands/models Dec 13, 2020
@jef jef merged commit 579cb97 into jef:main Dec 13, 2020
@github-actions github-actions bot mentioned this pull request Oct 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jef jef jef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kin3tik @jef