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.

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

send button -> split button #4924

Conversation

jackkav
Copy link
Contributor

@jackkav jackkav commented Jul 1, 2022

motivation: seems this component might be getting some work soon with the first websockets pass, so it would be nice for it to have good hmr and be a bit easier to reason about.

todo

  • fc conversion
  • split button
  • timer simplification
  • styling
  • hotkey behaviour

image

changelog(Improvements): Changed the Send Request button into a split-button to tell apart the basic send from other advanced send options

@jackkav jackkav self-assigned this Jul 1, 2022
@jackkav jackkav force-pushed the feature/ins-1567-send-button-hidden-menu-should-be-a branch 2 times, most recently from 1ecfa44 to 01ccf1f Compare July 1, 2022 11:19
@jackkav jackkav marked this pull request as ready for review July 1, 2022 11:19
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

it's a bummer that we have to achieve this by making a custom one-off class like .urlbar__send-context.

but all the same, in this styling it is absolutely critical that we have a border between the split. to many designers even that is not enough (please see elastic/eui#4171 (comment)) but we at least need that much.

please also read through all of https://www.nngroup.com/articles/split-buttons/ and make sure we have what we need for accessibility and usability.

@dimitropoulos
Copy link
Contributor

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

  1. we need to add segment events for this so we can see how much people are using it. specifically, we need to know when the dropdown is opened, but it'd also be important to be able to measure "clicked accidentally". I think we could do this by attaching events to each of the items as well. That's also important because we want to know about any of the items inside and when they are clicked. @wdawson please confirm that "event for opening the dropdown" + "no followup event soon after" is sufficient with analytics to discern accidental clicks. If not then perhaps we can just attach a followup event for dismissals and leave it at that.

  2. did you look at this in every theme? It's a pretty harsh/distracting line in some of them because it doesn't match or even look similar to any other border or separator on the screen. To be specific, you used 1px solid var(--color-bg) and there is literally no other active code path in the app that uses --color-bg for a border color (aside from what you just added). Almost all borders use the --hl prefixed colors, so maybe we should try some of these. I worry that there are no other places in the app that have a border on top of the surprise color, like this.

    Screenshot_20220701_164152

    Screenshot_20220701_164214

    Screenshot_20220701_165549

  3. I realize we're doing this without a design spec, but since this is such an extremely prominent thing in the app (in terms of functionality and visually) I think we should be cautious. What would you think about making padding-xs instead of padding-sm? It feels a lot more out-of-the-way to me. Regular sending is by-far the hottest use-case so we want to make sure that's what gets the lion's share of the space.

    BEFORE AFTER
    Screenshot_20220701_170132 Screenshot_20220701_170226

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

LGTM, most functionality seems ok.

Agree with what @dimitropoulos mentioned above, we might want to have a deeper look at how this handles themes.

On the other hand, taking into account what @dimitropoulos also posted internally about issues in themes perhaps we want to leave that study for another PR to properly fix themes and stuff that has been untouched since 3-4 years ago 😅

⚠️ Also found a few problems when we click on the send button or on the dropdown button:

  • We leave both the send button and the dropdown in a weird style after a click (similar to when we are hovering the element) and that "hover" like style only disappears if we click elsewhere:

Screenshot 2022-07-04 at 12 31 32
Screenshot 2022-07-04 at 12 31 24

  • We used to show a pointing hand on the Send button. Now we only show it on the dropdown next to send:
    image

image
image

@jackkav
Copy link
Contributor Author

jackkav commented Jul 4, 2022

we need to add segment events for this so we can see how much people are using it. specifically, we need to know when the dropdown is opened, but it'd also be important to be able to measure "clicked accidentally". I think we could do this by attaching events to each of the items as well. That's also important because we want to know about any of the items inside and when they are clicked. Wils Dawson please confirm that "event for opening the dropdown" + "no followup event soon after" is sufficient with analytics to discern accidental clicks. If not then perhaps we can just attach a followup event for dismissals and leave it at that.

Can be addressed in follow up PR.

@jackkav jackkav force-pushed the feature/ins-1567-send-button-hidden-menu-should-be-a branch from 33bdd3d to 6bef125 Compare July 4, 2022 15:18
@jackkav jackkav enabled auto-merge (squash) July 4, 2022 15:56
@jackkav jackkav merged commit c46d561 into Kong:develop Jul 4, 2022
@jackkav jackkav deleted the feature/ins-1567-send-button-hidden-menu-should-be-a branch July 4, 2022 16:05
@wdawson
Copy link
Contributor

wdawson commented Jul 5, 2022

Re: segment stuff

I don't think we need an event for "opened dropdown" There are cheaper, qualitative ways to analyze if people open it by mistake. What would be valuable is how often people are using the actions inside the dropdown, however. So events for the specific actions inside would be nice to have.

I agree a follow up PR makes sense but I also don't think it's urgently required.

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.

4 participants