-
Notifications
You must be signed in to change notification settings - Fork 22
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
Feature: Modal to allow users to list directly from inventory to CSFloat #288
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing to the CSFloat extension! We appreciate community contributions and definitely wasn't expecting a substantial PR.
I've added a couple comments, some in relation to code style and others for the UX -- it'd be great if we could try to match a similar UI style to either Steam or CSFloat for the model.
Got to most of it today -- will have time tomorrow to address UX/UI issues, CSS organization, and CSFloat auth state checking. |
Got caught up with work -- still getting this done. Had issues logging in last night. btw I noticed that the API lists valid auction durations as 1, 3, 5, 7, and 14 but (iirc) the csfloat site only allows for 1, 3, 7, or 14. Also, ran into an issue with listing prices -- what do you think about including subtotal & sale fees? Is there an easy way to fetch a user's sale fee %? cc @Step7750 |
Hey @IzaakPrats, thank you for the great contribution! You should be correct regarding the auction options, I believe the API docs are just outdated here. |
7ecff97
to
dfb9291
Compare
Requested review again and updated PR comment with new e2e tests |
60657fb
to
d7c1c4e
Compare
Really like the current version already! I do think we should explicitly handle Dopplers though. Instead of using the standard |
@GODrums -- nice! didn't know that endpoint existed. got it in. |
Thank you, looks great! Just tested it myself and noticed that in some cases the font-family is not applied correctly due to Steam's default CSS, for example the input and button elements still use Steam's original font-family. We likely have to create a manual rule to overwrite it for these elements. |
@GODrums - good catches! got both of those issues handled. Added a css override at the modal content level and made a new way for unauth exceptions to bubble up to the modal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for this huge PR!
Tested on Chrome and works.
Tested also on Firefox, but the /listings
endpoint doesn't support moz-extension origins yet (@Step7750 should probably be fixed server-sided?)
This is awesome! I'll try to get to re-reviewing this tomorrow -- feel free to ping me if I don't get to it. @GODrums I can allow-list that endpoint for the Firefox extension, I'll set a reminder before this goes live. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @IzaakPrats!
I've added a few comments, but we're very close to getting this merged. As mentioned briefly earlier, it's rare to see a quality PR like this from a community contribution -- so thank you again.
@@ -50,4 +52,6 @@ export const HANDLERS_MAP: {[key in RequestType]: RequestHandler<any, any>} = { | |||
[RequestType.FETCH_OWN_INVENTORY]: FetchOwnInventory, | |||
[RequestType.CANCEL_TRADE_OFFER]: CancelTradeOffer, | |||
[RequestType.FETCH_STEAM_TRADES]: FetchSteamTrades, | |||
[RequestType.LIST_ITEM]: ListItem, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should make the LIST_ITEM
and ListItem
scoped with CSFloat since it could also mean listing the item on Steam.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense but it's kind of already scoped by being in this extension -- not sure this would ever wrap the actual steam listing functionality since listing via extension is only csfloat. Happy to make the change if you think it would help clarify tho
…les fee section. Clean up some hardcoded consts
…lean up css a bit.
b005282
to
7d4ce04
Compare
Got those nits in @Step7750 by the way, do you have logging on extension usage? I'd be curious to know how many people use the extension to list and in the future, how many people use the Search on CSFloat button that go on to purchase. |
Looks like I missed a few comments. Will address those this week |
…her types in keys
7d4ce04
to
f12284c
Compare
Issue #277 List from Inventory
This PR adds the ability to list items directly on CSFloat from the Steam inventory. Users can create both buy now and auction listings with a streamlined interface.
Features
Implementation Details
FetchRecommendedPrice
handler to get price suggestions from CSFloatListItem
handler to create listings via CSFloat APIListItemModal
component with:Testing
No build / run errors
Buy Now Happy Path
csfloat_buy_now_happy_path.mov
Auction Happy Path
csfloat_auction_happy_path.mov
Not Logged In
csfloat_not_logged_in.mov
Price Fetch Failed
csfloat_cant_fetch_price.mov