Skip to content

Conversation

@rpierce99
Copy link

This pull request contains changes that are also in #140 since it builds on those changes, hopefully once that is merged this PR will be narrower in scope to just the counter part.

I'm by no means a designer, but hopefully this isn't too terrible looking
image

When counters are disabled the board looks normal, when counters are enabled they take up the lower 20% of each space and refit the text to accommodate.

The counters do not persist and are not shared with other players, they are an in-browser state only and would be lost after a refresh (similar to stars).

Thanks!

@kbuzsaki
Copy link
Owner

kbuzsaki commented Apr 4, 2021

Neat, thanks for having a go a this :)

I agree with the approach of having the counters as in-browser state, I don’t think this needs to be synced and I’d rather avoid the resulting complexity and data size. If the lack of persistence is a problem for people, I’d prefer to use browser local storage.

@armuell
Copy link

armuell commented Apr 4, 2021

My only feedback from a design/ux perspective is to consider reducing the space between the numbers and the buttons. With them at the edge of the square like that a user might misclick a button in a neighboring square which could get annoying. Counters reaching triple digits will be rare and anything more than that would be very surprising so I think having the buttons just outside the width of "999" (or whatever number is the widest) might be good.

This and the images PR are looking really awesome :)

@donsutherland
Copy link

donsutherland commented Apr 4, 2021

Nice work on both parts of this.

I think the counters a great but I do think that having them in sessionStorage or localStorage would be a nice feature in case a refresh is needed for whatever reason.

I also wonder if maybe a flag for whether counters applicable on the entries would be nice. For instance "Kill a Black Hinox" doesn't need a counter so why clutter the screen with it.

I did a review of the code also and it looks nice and clean.

@kbuzsaki
Copy link
Owner

kbuzsaki commented Apr 4, 2021

I think the implementation as-is is a good minimum viable product to send out. Local storage might be nice to have in the future, but stars don't have that today either and people make do.

I'm not sure that I'm on board with only showing the counters for certain goals. I like that this approach doesn't require any changes to the goal list (or heuristics to detect whether something needs a counter). I think it's also more flexible in case someone wants to use the counters in an unconventional way. If this approach feels too visually cluttered right now, then I think it'd be better to address that directly, e.g. by only showing the + button at first until the counter has a non-zero value.

@rpierce99
Copy link
Author

This is what the +/- hit box looks like currently
image
I'd be willing to move them closer to the counter if that's the consensus, but with the border in between the squares providing buffer and wanting to preserve as much space as possible for the "goal complete" hitbox I thought corners would be easy to "find" in the heat of a match.

@rpierce99
Copy link
Author

As far as a lighter visual treatment, we could narrow it down to just showing the + button at first, hide both the - and the counter itself until someone hits + on a given square. Potentially that could be done without needing to squeeze the rest of the content by 20% at first, and maybe then having it enabled by default would be the play.

@rpierce99
Copy link
Author

@kbuzsaki - just want to make sure you're not waiting on me for anything in these 2 PRs. I'm unfamiliar with your policies on merging, comment period, etc. Please let me know if there's anything else you'd like from me, and if not then what I should expect as far as merging. Thanks again.

@kbuzsaki
Copy link
Owner

@rpierce99 thanks for checking in and thanks for your patience :)
I'm not waiting on anything from you for these, I just haven't had a good time to merge and test them yet due to some life and work things in the past month. I'm hoping to have that time in the next week or so.

@rpierce99
Copy link
Author

No worries, hope the life and work things haven't been bad things! Appreciate the work you do for the community.

@kbuzsaki
Copy link
Owner

Hi rpierce, were you hoping to have one or both of these PRs merged in time for use with Bingothon? I was just reminded that that's coming up soon. Trying to balance the benefit gained from deploying the features beforehand versus the risk of added instability that may not have been debugged yet.

@rpierce99
Copy link
Author

@kbuzsaki thanks for asking, I don't personally run bingo and am not associated with bingothon, but I posted the question to the BOTW discord to let them know the question was asked. I expect we'll see one or more comments here with an answer, but I don't personally have an opinion either way.

@Floha258
Copy link

Hi rpierce, were you hoping to have one or both of these PRs merged in time for use with Bingothon? I was just reminded that that's coming up soon. Trying to balance the benefit gained from deploying the features beforehand versus the risk of added instability that may not have been debugged yet.

I am not the main dev when it comes to Bingosync features so take this with some grain of salt, however I am slightly scared of this potentially breaking some stuff in our system, depending on how this is implemented, especially when it comes to images. If image goals still have an alternate text attribute then it should probably be fine.

@rpierce99
Copy link
Author

I am not the main dev when it comes to Bingosync features so take this with some grain of salt, however I am slightly scared of this potentially breaking some stuff in our system, depending on how this is implemented, especially when it comes to images. If image goals still have an alternate text attribute then it should probably be fine.

I think you said bingosync and meant bingothon? Either way you can see the changes in #140 here
https://github.com/kbuzsaki/bingosync/pull/140/files#diff-c45cab67fbdb91ab8d04eedc1b74402f8269c65508cb24d38efb7d1037020270
Basically a new node is added for the image, alongside the text node, the image gets a "title" attribute which is what browser use for hover tooltips, and which node is visible is dependent on the setting on the right side. If you have additional questions about that PR we should probably move the discussion over there.

@armuell
Copy link

armuell commented May 26, 2021

I'll chime in since I'm also a restreamer for bingothon. I think just to play it safe both #140 and #141 should wait to be merged-in in the unlikely event some weird edgecase bug pops up. But #149 being just a goallist change would ideally be merged in so it can be showcased in the event.

@swiffy22
Copy link

I just did a normal Korok blackout on stream yesterday, and keeping count of everything I had done was the most troublesome part of the experience. I pulled up a notepad to try and keep track of the types and regions I had done, but an in-browser counter would be a much more elegant and less time-consuming solution.

@lepelog
Copy link
Contributor

lepelog commented Aug 22, 2021

There is now an instance of bingosync using this branch live at https://bingosync.bingothon.com, if anyone wants to test it out and give feedback

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.

7 participants