Skip to content
This repository has been archived by the owner on Oct 16, 2023. It is now read-only.

Feature: add focus ring for blocks #211

Merged
merged 10 commits into from
Aug 2, 2023
Merged

Conversation

glaubersilva
Copy link
Contributor

@glaubersilva glaubersilva commented Jul 7, 2023

Description

This PR adds a focus ring for the blocks when they are selected. This feature is helpful because provides UI feedback about which block is selected.

Affects

The form builder

Visuals

image

image

Testing Instructions

Click on any block in the form builder and you should see a blue focus ring indicating that the block is selected, check the attached screenshot for reference.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@glaubersilva glaubersilva self-assigned this Jul 7, 2023
@glaubersilva glaubersilva marked this pull request as ready for review July 7, 2023 20:03
@glaubersilva glaubersilva requested a review from jonwaldstein July 7, 2023 20:20
@glaubersilva
Copy link
Contributor Author

@jonwaldstein Thanks for the explanations, now I better understand your concerns in regard to this. I did implement your suggestion with a slight difference, let me know if there is anything else.

@jonwaldstein
Copy link
Contributor

@glaubersilva thanks! looks good to me but i'm also not the designer. I'm thinking since this will affect all block selections we should do a quick screen share with @jdghinson to get his thoughts. I think in his earlier designs he had a selected ring but there was no padding - so we'll want to see how this interacts with different block types.

@glaubersilva
Copy link
Contributor Author

@jonwaldstein Thanks! I already create a new zip with these last changes and send it to @jdghinson via Slack for a re-review.

@glaubersilva
Copy link
Contributor Author

@jonwaldstein After @jdghinson reviewed it, we implemented a couple of cosmetic changes, but one specific required a bit of JS. So, I recorded a video to better explain why it was necessary, let me know your thoughts on it. Thanks!

Video: https://www.loom.com/share/6031260278924155b4289a856a0417d1

@glaubersilva
Copy link
Contributor Author

@jonwaldstein This is ready for re-review.

Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

@glaubersilva great job man!

@glaubersilva glaubersilva merged commit ca4654d into develop Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants