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

As a user, when clicking on the 'enter' button to load a room/venue, I want to be able to choose to open a new tab (aka: replace button with a href) #1670

Open
0xdevalias opened this issue Jun 27, 2021 · 6 comments
Open

As a user, when clicking on the 'enter' button to load a room/venue, I want to be able to choose to open a new tab (aka: replace button with a href) #1670

0xdevalias opened this issue Jun 27, 2021 · 6 comments
Labels
blocked-by-tech-debt This issue is blocked by existing tech-debt. 💪 enhancement Enhancements/improvements to existing functionality 🏎️ performance For performance related issues/improvements 💥 tech-debt ✨ UI/UX For UI/UX related issues, let's make the platform and customer experience sparkle! ✨ 🔢 counting For issues related to the 'counting' feature

Comments

@0xdevalias
Copy link
Contributor

0xdevalias commented Jun 27, 2021 •
edited
Loading

This was raised most recently (to my knowledge) by @yarikoptic in #1387

Such UX (ability to open a room in a new tab) is desperately needed, in particular
for poster rooms etc, since opening a room can take considerable time of a blue window
of sparkle.

Given there is a bit more nuance/consideration required to resolve this, and some related tech debt around how having multiple tabs/browsers open at once affects the user presence/'counting' feature, I wanted to create an issue to capture/centralise it all a bit better. There is a bunch of discussion on that PR in comments/etc, but it might be hard to follow, or too easy to lose there.
@0xdevalias 0xdevalias added 💪 enhancement Enhancements/improvements to existing functionality 🔢 counting For issues related to the 'counting' feature 💥 tech-debt ✨ UI/UX For UI/UX related issues, let's make the platform and customer experience sparkle! ✨ 🏎️ performance For performance related issues/improvements blocked-by-tech-debt This issue is blocked by existing tech-debt. labels Jun 27, 2021
@0xdevalias
Copy link
Contributor Author

I can't remember specifically off the top of my head, but I believe we used to use <a href tags for these sorts of features, but explicitly changed it to use <button> to avoid the ability for people to open multiple new tabs.

The way the 'user presence' (aka: 'counting', aka: 'where people are shown as being on the map/within the platform') system works at the moment doesn't really support having multiple browser tabs open and connected to Sparkle at the same time.

I would have to check in with the team (@mike-lvov @denisdimitrov FYI) further about this, and whether it's a change we would be ok letting back into the platform given its impacts on 'counting' and other features.

Originally posted by @0xdevalias in #1387 (review)

From a quick skim of the code, it also seems like if we were to replace the <button> with an <a href outright, we may run into some potential issues with the enterRoomWithSound feature:

And would need to change the logic around how enterRoom works currently, to provide the appropriate props/etc to open internal vs external links:

Originally posted by @0xdevalias in #1387 (comment)

I wonder if some "onclick" could event be assigned to a

I believe this is possible, though i'm not aware off the top of my head if it's 'bad practice'. Semantically, (usually), <a href is used for links and such, and <button>s are used for adding various JavaScript event handlers when clicked.

Originally posted by @0xdevalias in #1387 (comment)

@0xdevalias
Copy link
Contributor Author

0xdevalias commented Jun 27, 2021 •
edited
Loading

I can't remember specifically off the top of my head, but I believe we used to use <a href tags for these sorts of features, but explicitly changed it to use <button> ...

if you could point to commit/PR which switched from a to button - might help to 'harmonize` this PR a bit:

... to avoid the ability for people to open multiple new tabs. The way the 'user presence' (aka: 'counting', aka: 'where people are shown as being on the map/within the platform') system works at the moment doesn't really support having multiple browser tabs open and connected to Sparkle at the same time.

it seems to be the actual issue, which needs to be addressed:

  • users will (and should) be able to have multiple windows/tabs open even if by manually copy pasting urls
  • users will share (twitter etc) URLs to their poster rooms, thus facilitating that even if sparkle tries to "lock it down"
  • preventing multiple tabs/windows (as "use button instead of a) would not prevent that and just contribute to bad UX: it should be up to a user to either open in a new tab/window or just follow within existing one

I do appreciate tech side additional complexity of implementing proper 'counting' but it is doable. Is there a dedicated issue for it ? we might be able to help . E.g. see how @soichih did it last year for poster sessions: https://github.com/datalad-datasets/ohbm2020-posters/blob/gh-pages/ui/src/components/Room.vue#L53 -- may be it would give extra usable info. But in general it should be overall quite doable: just send heartbeat from clients e.g. each 2 minutes, and send events when leaving the page or tab being closed (iirc there is support to subscribe to that event as well -- and it was done in above project somewhere).

Overall -- I would consider "wrong count" less severe issue than inability to open e.g. a poster room in a new window. This would complicate "attend to your poster while visit others until some visitors arrive" use case and others.

Originally posted by @yarikoptic in #1387 (comment)

if you could point to commit/PR which switched from a to button - might help to 'harmonize` this PR a bit:

@yarikoptic Don't know off hand, but I would be starting by looking at the commit history for this file:

As well as the 'blame' view:

Stepping back from that blame view gave me this version, which is using an <a href tag:

Looks like it changed from <a href> to button` in #810

users will (and should) be able to have multiple windows/tabs open even if by manually copy pasting urls

@yarikoptic While this is definitely possible now, some potential future work (eg. one idea that is on our backlog for it) would have the platform detect and prevent this by showing the user a specific message

preventing multiple tabs/windows (as "use button instead of a) would not prevent that and just contribute to bad UX: it should be up to a user to either open in a new tab/window or just follow within existing one

@yarikoptic I don't disagree with you. It's a matter of trying to balance our engineering capacity across all of the things that need to be done I suppose. At the time, that was the best solution we had capacity for, and we went with it.

I do appreciate tech side additional complexity of implementing proper 'counting' but it is doable.

@yarikoptic Agreed.

Is there a dedicated issue for it ? we might be able to help

@yarikoptic We have a couple of scattered backlog issues that mention counting ([sparkle-internal-ref] 🔒 https://github.com/sparkletown/internal-sparkle-issues/issues/196, counting label, etc), but I don't think there are any specific ones dedicated to the aspects we've been talking about here.

I couldn't tell you if this is all of the bits of code off the top of my head, but I would start by looking in utils/userLocation + places that use that:

You can see the code that sets the location when a user enters a venue in VenuePage:

As well as (one of?) the 'heartbeat' type functions:

As well as the code that clears the user location when the window/tab closes:

Hopefully that gives you enough pointers to start figuring out where it all is and thinking through potential improvements/solutions.

If you come up with anything that would work, I would definitely be keen to hear your thoughts on it! If you do, probably would be good to have a chat about it in #external-open-source on Slack/similar before you get too deep into making all of the code changes, just so we can validate the 'plan' to assure it aligns for us before you end up spending too much technical time on it.

But in general it should be overall quite doable: just send heartbeat from clients e.g. each 2 minutes, and send events when leaving the page or tab being closed (iirc there is support to subscribe to that event as well -- and it was done in above project somewhere).

@yarikoptic This is actually pretty close to what we do currently. But if a user has multiple tabs open in multiple venues, all sending the "I am here" heartbeat, where should the system consider that user to be? The current implementation is built on the assumption that a single person will only be in a single place at a single time. So to receive these multiple conflicting heartbeats, they will end up arbitrarily 'jumping around' based on which heartbeat updated most recently (which is basically what happens currently if you manually force multiple tabs by directly browsing to the URLs/etc)

For most 'less technical' users, the additional 'hinderance to opening a new tab' that the button introduced meant that for most cases, users wouldn't bother, and so we would work around that issue for the majority of users (if the odd person here or there does it, it won't dramatically affect the counting/presence, but if everyone is bouncing around all over the place, it'll have a pretty big impact on the UX of that feature).

So at the end of the day, based on the current implementation, it's basically one form of UX tradeoff against another. We chose to prioritise the UX of the counting/presence feature, as it's quite important for most events to see "what is happening where", and to find other users/etc rather than ending up in a room alone by yourself.

Overall -- I would consider "wrong count" less severe issue than inability to open e.g. a poster room in a new window. This would complicate "attend to your poster while visit others until some visitors arrive" use case and others.

@yarikoptic As mentioned above, it's a case of trading off one 'better UX' against the other based on the limitations of the current setup; and for most events we've hosted/run, counting/presence is a far higher priority than the ability to easily open multiple tabs.

For now I would suggest that perhaps just manually using the URL and copy/pasting to get a new tab should be a workable workaround for that use case.

Originally posted by @0xdevalias in #1387 (comment)

Thanks for all the pointers. I hope that may be @soichih could have a look at his spare time. I will also think about it for possible ideas. E.g. may be it is possible to send heartbeat only from "currently focused on" window -- so indeed allow for a singular location being tracked while user might potentially have multiple windows opened etc. Indeed user would then "jump around" but that would actually be reflective of "user jumping around" ;)

edit1: this would nohow address a use case of having user logged in from multiple computers but IMHO that is ok, and would still be improvement.

As for

users will (and should) be able to have multiple windows/tabs open even if by manually copy pasting urls
@yarikoptic While this is definitely possible now, some potential future work (eg. one idea that is on our backlog for it) would have the platform detect and prevent this by showing the user a specific message

please don't ;)

For now I would suggest that perhaps just manually using the URL and copy/pasting to get a new tab should be a workable workaround for that use case.

that is where we agree to disagree since IMHO it wouldn't anyhow solve tracking issue and just make sparkle harder to use for upcoming OHBM event .

Originally posted by @yarikoptic in #1387 (comment)

I will also think about it for possible ideas. E.g. may be it is possible to send heartbeat only from "currently focused on" window -- so indeed allow for a singular location being tracked while user might potentially have multiple windows opened etc. Indeed user would then "jump around" but that would actually be reflective of "user jumping around" ;)

@yarikoptic At face value, that could be a decent way to go about solving that aspect of things. I would have to sit down and think through the system at a deeper level to assess if that would cover all related cases effectively.

edit1: this would nohow address a use case of having user logged in from multiple computers but IMHO that is ok, and would still be improvement.

@yarikoptic 👌🏻

some potential future work (eg. one idea that is on our backlog for it) would have the platform detect and prevent this by showing the user a specific message

please don't ;)

@yarikoptic 😆 Haha, yeah. Personally I agree that it would be suboptimal UX. (cc // @mike-lvov @jonboutelle @sparkletown/product FYI RE: 'blocking users from being able to have more than 1 sparkle tab/window open at the same time')

For now I would suggest that perhaps just manually using the URL and copy/pasting to get a new tab should be a workable workaround for that use case.

that is where we agree to disagree since IMHO it wouldn't anyhow solve tracking issue and just make sparkle harder to use for upcoming OHBM event .

@yarikoptic The nuance of a 'workaround' is a solution that requires no deviation/changes to the current platform code as it exists right now.

While it's not a solution to the counting problem, nor the overall UX, it is a method of solving the "I want to open multiple tabs" situation right now, without any code changes; and is implementable at an individual user level without needing to be 'blocked' on assessing the wider impacts across all users that changing the default capability requires.

Originally posted by @0xdevalias in #1387 (comment)

@0xdevalias
Copy link
Contributor Author

I am just going through this thread. I agree with @yarikoptic that people will do open multiple tabs during the conference (or just hit Alt+D+Enter to duplicate a tab to open new one)

To understand what needs to be updated.. are the following assumptions correct?

  1. When I open multiple tabs, the useUpdateTimespentPeriodically correctly reports the visit stats on all rooms. So nothing needs to be done here.
  2. The user.lastSeenIn is updated by setInterval on the VenuePage using the current profile.lastSeenIn, so no matter how many tabs are opened, it will be set to the last page that user entered (it won't be flip / flopping the location or anything like that). I think this is an expected behavior, so nothing need to be done here either.
  3. I saw some mentioning of "sound" but I don't hear any sound .. so I don't quite understand why it's discussed here.
  4. When I close a tab, it fires clearLocationData and removes lastSeenIn from user profile. The remaining page then starts reporting "undefined":string (a bug?)

image

To fix this "bug", and at the same time fix the issue #4, we could simply update updateCurrentLocationData() so that if it tries to set the lastSeenIn to undefined, we will first reset the user profile to use the current venue ID of a surviving tab, then start reporting on that lastSeenIn. I think it's reasonable to assume that a user is "active" on one of the remaining tab.

Is there any other consideration that I am missing here?

So in summary, all we have to do is to fix #4 (easily) and we should have a pretty solid handling for multiple tabs.

As far as wrapping a v.s. button issue, can we not just use a naked a instead of button and apply all the button css on the a element to make it look like a button? I do that very often and it works fine. Users like @yarikoptic can then just right click and select "Open link in a new tab" option to open a new tab? Is that why @yarikoptic wants to use a?

Originally posted by @soichih in #1387 (comment)

@0xdevalias
Copy link
Contributor Author

Edit: I have also captured this on our internal 'Counting Logic' issue in 🔒 https://github.com/sparkletown/internal-sparkle-issues/issues/196#issuecomment-867293968

RE: #1387 (comment)

Sorry this took me so long to get back to commenting on (being pulled in too many directions + executive dysfunction combined to make this feel 'too big' to try and comment on and ensure I was giving the correct advice/answers), probably a bit late for the conference now, but for future benefit, will try and answer as best I can.

I've also started some transitively related work on lastSeenIn / lastSeenAt in #1623 + any followup PRs. So what i'm saying here should be true right now, but may not remain perfectly so depending on how that work progresses.

To understand what needs to be updated.. are the following assumptions correct?

  1. When I open multiple tabs, the useUpdateTimespentPeriodically correctly reports the visit stats on all rooms. So nothing needs to be done here.

@soichih Looking at the code just now, with the caveat of the @debt comment listed there (see below), that seems correct to me. Each will update its own locationName document, and they use firebase.firestore.FieldValue.increment so there shouldn't be any race conditions.

If the same person is in the same venue, then the time spent will be inaccurate, as all tabs will be updating it:

@debt time spent is currently counted multiple times if multiple tabs are open

  1. The user.lastSeenIn is updated by setInterval on the VenuePage using the current profile.lastSeenIn, so no matter how many tabs are opened, it will be set to the last page that user entered (it won't be flip / flopping the location or anything like that). I think this is an expected behavior, so nothing need to be done here either.
  • src/pages/VenuePage/VenuePage.tsx calls updateCurrentLocationData within a useInterval and passes the profile location (now called userLastSeenIn with the changes in [env/ohbm] refactor lastSeenAt / lastSeenIn out of main User document + associated refactors #1623) as you said above.
    • ⚠️ Note, this is probably not ideal, as I expect there will be a potential race condition here where we could be passing through old/invalid data. We sometimes end up seeing the lastSeenIn location get set to undefined. I wonder if that relates in some way to this.
  • updateCurrentLocationData is defined in src/utils/userLocation.ts, and extracts locationName from profileLocationData using const [locationName] = Object.keys(profileLocationData);
    • ⚠️ This seems even more likely to be the location/source of the undefined bug mentioned above..
  • This then uses updateLocationData to set newLocationData to { [locationName]: getCurrentTimeInMilliseconds() }
    • ⚠️ Because the user's existing location from their profile is passed in here, and then it will just keep getting updated, there are a few probable bugs/issues here with multiple tabs:
      • This is built on the assumption that the last location on your profile is the location you're currently in (not necessarily true, and somewhat introduces a race condition between this code + the code that changes the location you're in
      • Each tab that is open will be running it's own version of this interval, and so will be updating this field, so we're basically going to needlessly multiply the number of writes to the database based on tabs open, which will also cause this updated data to flow through to every client multiple times. Which in a normal situation shouldn't be the biggest problem in the world, but since one of the core performance issues (see [env/ohbm] refactor lastSeenAt / lastSeenIn out of main User document + associated refactors #1623) seems to be related to user location updates, and that invalidating all of the memo'd collections/etc on the frontend (which are used all over the place, so ends up causing a whole bunch of extra re-rendering, etc), this is of greater concern here.
  • updateLocationData is defined in src/utils/userLocation.ts, receives the locationData record to update, and then calls updateUserProfile with it to set both lastSeenAt and lastSeenIn
    • Since this function doesn't do a whole lot, I won't comment more here, but I expect the next one will highlight another issue here..
  • updateUserProfile is defined in src/pages/Account/helpers.ts
    • 💥 TODO[TECH-DEBT]: This should probably be refactored into the api/* layer of things at some point
    • It uses firestores .update() function (ref), and basically passes in the entire profileData params it receives, which is passed from updateLocationData, and is basically an object with the lastSeenAt + lastSeenIn keys (as described above)
    • ⚠️ Because lastSeenIn is an object, the way it is passed in/updated here, it will be entirely overwritten each time it is updated. I believe this is 'expected behaviour', but I wonder if there are some nuances that could hurt us here. At a minimum, with each new tab overwriting this field, we'll have a lot more updates than we expect (see issue described above), and since the current code will only store a single location here, the user is only going to be shown as being in 1 location (the last one that was set), yet every open tab is going to be updating this location's lastSeenIn time, which means that we may show someone as still being present in that location even long after they have left, so long as they are still 'somewhere else' on the platform.
      • My memory of this feature is that it used to be designed to be somewhat tolerant of being in multiple locations (albeit in a convoluted and confusing way), but I think that sort of got lost/regressed in the code somewhere along the way.
      • We're not super happy with the current presence method as it's rather ephemeral, and doesn't allow us to answer questions/provide richer analytics questions such as "How did user's move about through the party/event?", which we have been asked for in the past. While we haven't spent a lot of time to dive into it and properly design/architect the next iteration of it, off the top of my head, it would make sense to me for it to be a sub-collection on off the user object or similar, and for us to keep 'ongoing records' of the time they entered a particular venue/similar. Off the top of my head, this might be structured as 1 document in this subcollection per venue visited, and then maybe an array within that for the timestamp of when they entered or similar. (note: I haven't thought through exactly how this 'keep their lastSeenIn time up to date within this venue' would work in that model, we definitely wouldn't want to constantly push new times into that array, so the document may have a separate field for that or similar maybe?). This would quite likely be combined into our existing visits sub-collection or similar, which currently just holds the timeSpent count.
  1. I saw some mentioning of "sound" but I don't hear any sound .. so I don't quite understand why it's discussed here.

@soichih You can see the discussion + details in #1387 (comment) (including links to code)

Basically the platform can be configured to play a sound effect when entering a room.

The main parts of this are defined in src/hooks/sounds.tsx, but the useCustomSound hook is basically all we need to know/care about for the specifics here.

In RoomModalContent (defined in src/components/templates/PartyMap/components/RoomModal/RoomModal.tsx)

  • we call useRoom to get the enterRoom function
  • we call useCustomSound with the onend parameter set to enterRoom, which returns us enterRoomWithSound, which will play the sound (if any is configured), and then call the onend function (eg. enterRoom)

You can read the discussion above for full details/context, but basically, given we're currently using a <button> it's easy/canonical to call a function in the onClick, yet when using an <a>, the concern I raised was around whether it's semantically correct to add an onClick to it, along with how to handle any accessibility concerns that may come up with that, while still being able to use the 'play sound effect' part of the code.

When the feature was implemented, we didn't use 'in-page' React-Router based navigation at all, and so loading a venue was a full new page load (this is something we want to change, and have started to in some places, but not everywhere yet as there are some concerns/associated risks that we want to be mindful of to ensure we don't break things when we do it). That meant that as soon as we start navigating to the new venue, our page unloads, and thus sound/etc can't be played. When implementing this feature, I decided to ensure the sound plays before we start the navigation, to ensure this works as expected. When we update the platform to use React-Router for all navigation, I expect this will be less of a concern in most instances, but it will still be something we need to be mindful of when a room is linked to an external URL, as that will essentially have the same 'issues' to be worked around as what we currently have (eg. can't play a sound when the page is unloaded)

That's a lot of background/context for the 'why', but basically, as it pertains to this particular PR/issue being discussed, we basically need to ensure we can use an onClick on an <a> tag in an accessible/canonically correct way, and be able to prevent the default page navigation from happening in those situations.

  1. When I close a tab, it fires clearLocationData and removes lastSeenIn from user profile. The remaining page then starts reporting "undefined":string (a bug?)

image

@soichih Definitely a bug! ⚠️🐛

To fix this "bug", and at the same time fix the issue #4, we could simply update updateCurrentLocationData() so that if it tries to set the lastSeenIn to undefined, we will first reset the user profile to use the current venue ID of a surviving tab, then start reporting on that lastSeenIn. I think it's reasonable to assume that a user is "active" on one of the remaining tab.

@soichih I think I kind of alluded to it in what I said above, but just in case I didn't, shall state it directly here. I think I don't really like our current distinction between setLocationData and updateCurrentLocationData, and i'm not really sure that I like the pattern we have of reading/passing in the existing userLocation from the profile + overwriting it entirely. At the very least, the current way this is laid out (pulling/passing data at the component level) is super hacky. If we need to do this, we should be doing it within a firebase transaction, and doing it at the api/* layer of things. We're too haphazard with how we let the 'implementation detail' of the data structure 'leak out' to this layer of things as well.

All that VenuePage should need to know/care about is that it's calling a function to say "the user is in this location", and to update that over time. Everything else should be 'encapsulated' and hidden way from the component layer. This function should probably just take the userId as well as the venueId as params, but right now we're using venueName for legacy reasons, so I guess it would need to start off being that unless we refactor both at the same time. It may need another param/similar if we also need to be able to handle 'rooms' in a different way.. need to check the deeper logic around this feature to know for sure)

By encapsulating these 'implementation details' in a single location and not 'exposing' that as part of the public API, we will also make it much easier to support different needs, even without refactoring the entire underlying architecture of it. Eg. we could make each tab only explicitly update the venue it is currently in, and not overwrite the others, which would help to support this 'multiple tabs' use case without breaking everything.

Is there any other consideration that I am missing here?

@soichih Described above as best as I can think through/figure the problem space right now.

As far as wrapping a v.s. button issue, can we not just use a naked a instead of button and apply all the button css on the a element to make it look like a button? I do that very often and it works fine. Users like @yarikoptic can then just right click and select "Open link in a new tab" option to open a new tab? Is that why @yarikoptic wants to use a?

@soichih I spoke to some of this above already (RE: accessibility, the onClick handler aspect, etc); as well as the general 'multiple tabs are unsupported because they break critical features' stuff described previously + in my comments above. The literal technical changes to be made here are rather easy, it's the nuance/flow on effects/etc that need to be properly considered and the change made mindfully/properly; rather than just 'hacking something in' and 'testing it in production'/etc.

Originally posted by @0xdevalias in #1387 (comment)

@0xdevalias 0xdevalias mentioned this issue Jun 27, 2021
Closed
@0xdevalias
Copy link
Contributor Author

0xdevalias commented Jun 27, 2021 •
edited
Loading

I wonder if some "onclick" could event be assigned to a

I believe this is possible, though i'm not aware off the top of my head if it's 'bad practice'. Semantically, (usually), <a href is used for links and such, and <button>s are used for adding various JavaScript event handlers when clicked.

Further to the accessibility thoughts/concerns, I stumbled across the following today:

@0xdevalias 0xdevalias mentioned this issue Jun 27, 2021
Merged
@whoaitsaimz
Copy link
Contributor

Hey @ermiaSparkle this issue is also from public repo, do you think this has been sufficiently addressed with our July accessibility updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocked-by-tech-debt This issue is blocked by existing tech-debt. 💪 enhancement Enhancements/improvements to existing functionality 🏎️ performance For performance related issues/improvements 💥 tech-debt ✨ UI/UX For UI/UX related issues, let's make the platform and customer experience sparkle! ✨ 🔢 counting For issues related to the 'counting' feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@0xdevalias @whoaitsaimz