feat(TextInputInnerAction): adds a tooltip-direction property to the … #3011

edersonlucas · 2023-03-10T02:53:43Z

Component: TextInputInnerAction

In the TextInputInnerAction component, the tooltip direction property defaults to 'n'. Therefore, I decided to add the tooltip-direction property to the component.

Motivation

Recently, in a project I was working on, we had a bug in the mobile screen mode with a password input that had a button to show the password. When the button was clicked, the tooltip that appeared overflowed beyond the screen, causing a horizontal scroll. For this reason, I am adding the tooltip-direction property to the TextInputInnerAction.

Changelog

New

Add tooltip-direction property.

Changed

Update TextInput.docs.json to add the tooltip-direction property.
Update the Storybook of the TextInput component to provide an example of how the tooltip-direction property would be used.

Removed

Screenshots

Before

After

Merge checklist

…component

changeset-bot · 2023-03-10T02:53:45Z

🦋 Changeset detected

Latest commit: 0ddad0c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

broccolinisoup

👋🏻 @edersonlucas

Thank you for taking the time to create this PR! The additional prop for the tooltip direction sounds good to me but I'm going to tag https://github.com/orgs/primer/teams/accessibility-design to see what they think. Thanks!

edersonlucas · 2023-03-13T03:00:02Z

@broccolinisoup Perfect, I'll be waiting for it.

ichelsea · 2023-03-13T16:36:29Z

Adding the extra prop of the direction adjustment shouldn't be a problem 🙂. It looks like we're using the updated accessible tooltip component (I think!) so that sounds good as well!

broccolinisoup · 2023-03-13T23:00:22Z

Thanks @ichelsea! The tooltip is still WIP for accessibility improvements https://github.com/github/primer/issues/1790 (GitHub stuff only link) hopefully should be completed soon 😊

broccolinisoup · 2023-03-13T23:01:59Z

@edersonlucas could you please add a changeset to the PR? https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md - feel free to have a look at the other PRs as well to get an idea of how we write changesets. Thanks!

edersonlucas · 2023-03-13T23:17:37Z

@broccolinisoup Thank you, I will analyze here how to make the changeset 😊

edersonlucas · 2023-03-14T00:03:36Z

@broccolinisoup I just made the changeset, I believe I did it correctly 😀

.changeset/wild-fishes-carry.md

docs/content/TextInput.mdx

src/TextInput/TextInput.features.stories.tsx

edersonlucas · 2023-03-15T01:07:42Z

@broccolinisoup Corrected 😀

broccolinisoup

Sorry for the late response! It looks good to me ✨ Thank you so much for your time to push that PR 🙏🏻

edersonlucas · 2023-03-28T23:14:47Z

@broccolinisoup Hello, I have a question. Is there any way I can fix this error in the "Deploy (fork) / Preview / Build (pull_request_target)" of Github Actions?

broccolinisoup · 2023-03-29T01:53:05Z

@edersonlucas Hi! I think they were expected to fail as this PR is coming from a fork but I'll confirm it with the team if it is okay to merge it with the fails. I'll get back to you tomorrow. Sorry for still not being able to merge this! Thanks for your patience 🙏🏻

edersonlucas · 2023-03-29T02:11:06Z

@broccolinisoup Ok, I understood. Thank you for the feedback 🙏🏻

broccolinisoup · 2023-03-30T01:26:10Z

@edersonlucas I think we are good to merge your PR! ✨

There is only one failing on the CI now and it is expected due to your PR is coming from a fork repo.
We fixed the other failing CI that was caused by peer dependency issues #3092

I am going to go ahead and merge your PR now. Thank you so much for your contribution! We really appreciate it 🙏🏻
Also for your info, we are currently working on remediating accessibility issues on the Tooltip component #3032 . Once it is released, TextInputInnerAction component will have an accessible tooltip as well ✨

edersonlucas · 2023-03-30T01:32:34Z

@broccolinisoup Hi! I was thinking, could this issue be because I directly modified the components.json? I say this because looking at other PRs, it's modified by Github Actions. Do you think if I revert it, it will pass the test?

edersonlucas · 2023-03-30T01:35:21Z

That's great, I'll take a look at #3032 and see if I can contribute in any way 😊

broccolinisoup · 2023-03-30T01:39:46Z

@broccolinisoup Hi! I was thinking, could this issue be because I directly modified the components.json? I say this because looking at other PRs, it's modified by Github Actions. Do you think if I revert it, it will pass the test?

It is failing because secrets (in this case, it is app_id) are not passed to workflows that are triggered by a pull request from a fork.
reference

edersonlucas · 2023-03-30T01:55:48Z

@broccolinisoup Thank you for your attention 🙏🏻 I recently started contributing to open-source projects here on GitHub and I would like to share the experience and insights I gained while contributing to Primer. I'm considering writing a post about it on LinkedIn. Would it be possible to mention you in my post?

broccolinisoup · 2023-03-30T22:12:14Z

@edersonlucas Thank you! You can mention me in your post. Would it be possible to share it with me before posting?

Also, it was brought to my attention that we have a pattern of using camel case prop names (tooltipDirection instead of tooltip-direction) and I totally missed this in your PR! Would you be keen to push another PR that renames the prop name to tooltipDirection ?

edersonlucas · 2023-03-30T22:19:44Z

@edersonlucas Thank you! You can mention me in your post. Would it be possible to share it with me before posting?

Also, it was brought to my attention that we have a pattern of using camel case prop names (tooltipDirection instead of tooltip-direction) and I totally missed this in your PR! Would you be keen to push another PR that renames the prop name to tooltipDirection ?

@broccolinisoup Thank you! Yes, I'll send it to you before posting. I'll look for you on LinkedIn.
Regarding the PR, I'll make the necessary corrections and submit another one 😊

broccolinisoup · 2023-03-30T22:58:18Z

@edersonlucas Thank you so much for your responsiveness. I appreciate it! You can find my linkedin on my github profile.

broccolinisoup · 2023-03-30T23:19:31Z

@edersonlucas We possibly need to do an urgent release so we wanted to make sure the tooltip direction prop is being released with the came case name. Please see the PR: #3101 We won't need another PR to rename the prop anymore. Thanks so much for your support and contribution on this work and sorry if there was any confusion!

edersonlucas · 2023-03-30T23:26:38Z

@edersonlucas We possibly need to do an urgent release so we wanted to make sure the tooltip direction prop is being released with the came case name. Please see the PR: #3101 We won't need another PR to rename the prop anymore. Thanks so much for your support and contribution on this work and sorry if there was any confusion!

Perfect! No need to apologize. I appreciate your attention and kindness once again!

feat(TextInputInnerAction): adds a tooltip-direction property to the …

f8a83b5

…component

edersonlucas requested review from a team and JoshBowdenConcepts March 10, 2023 02:53

edersonlucas added 2 commits March 10, 2023 00:34

Merge branch 'main' into add-tooltip-direction-in-textinputaction

85470d0

Merge branch 'main' into add-tooltip-direction-in-textinputaction

f39f56c

edersonlucas marked this pull request as draft March 10, 2023 17:22

edersonlucas marked this pull request as ready for review March 10, 2023 17:57

docs(TextInputInnerAction): update component documentation

2244b8f

edersonlucas force-pushed the add-tooltip-direction-in-textinputaction branch from 49afbc6 to 2244b8f Compare March 10, 2023 23:15

This comment was marked as duplicate.

Sign in to view

broccolinisoup reviewed Mar 13, 2023

View reviewed changes

docs(TextInputInnerAction): add changeset

f24f42d

edersonlucas force-pushed the add-tooltip-direction-in-textinputaction branch from 5192aad to f24f42d Compare March 13, 2023 23:31

edersonlucas added 2 commits March 13, 2023 22:21

Merge branch 'main' into add-tooltip-direction-in-textinputaction

b36a1f6

docs(TextInputInnerAction): update changeset

4712b44

broccolinisoup reviewed Mar 15, 2023

View reviewed changes

.changeset/wild-fishes-carry.md Outdated Show resolved Hide resolved

docs/content/TextInput.mdx Show resolved Hide resolved

src/TextInput/TextInput.features.stories.tsx Show resolved Hide resolved

docs(TextInputInnerAction): update docs

63b1dc7

Merge branch 'main' into add-tooltip-direction-in-textinputaction

69494df

broccolinisoup approved these changes Mar 22, 2023

View reviewed changes

edersonlucas added 2 commits March 22, 2023 14:04

Merge branch 'main' into add-tooltip-direction-in-textinputaction

49f9e99

Merge branch 'main' into add-tooltip-direction-in-textinputaction

3b83232

Merge branch 'main' into add-tooltip-direction-in-textinputaction

0ddad0c

broccolinisoup deployed to github-pages March 30, 2023 00:49 — with GitHub Actions View deployment

broccolinisoup added this pull request to the merge queue Mar 30, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 30, 2023

broccolinisoup added this pull request to the merge queue Mar 30, 2023

broccolinisoup removed this pull request from the merge queue due to a manual request Mar 30, 2023

broccolinisoup merged commit f8132d8 into primer:main Mar 30, 2023

primer-css mentioned this pull request Mar 30, 2023

Release tracking #3091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(TextInputInnerAction): adds a tooltip-direction property to the … #3011

feat(TextInputInnerAction): adds a tooltip-direction property to the … #3011

edersonlucas commented Mar 10, 2023 •

edited

Loading

changeset-bot bot commented Mar 10, 2023 •

edited

Loading

This comment was marked as duplicate.

broccolinisoup left a comment

edersonlucas commented Mar 13, 2023

ichelsea commented Mar 13, 2023

broccolinisoup commented Mar 13, 2023

broccolinisoup commented Mar 13, 2023

edersonlucas commented Mar 13, 2023

edersonlucas commented Mar 14, 2023

edersonlucas commented Mar 15, 2023

broccolinisoup left a comment

edersonlucas commented Mar 28, 2023

broccolinisoup commented Mar 29, 2023

edersonlucas commented Mar 29, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023 •

edited

Loading

broccolinisoup commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

feat(TextInputInnerAction): adds a tooltip-direction property to the … #3011

feat(TextInputInnerAction): adds a tooltip-direction property to the … #3011

Conversation

edersonlucas commented Mar 10, 2023 • edited Loading

Component: TextInputInnerAction

Motivation

Screenshots

Merge checklist

changeset-bot bot commented Mar 10, 2023 • edited Loading

🦋 Changeset detected

This comment was marked as duplicate.

broccolinisoup left a comment

Choose a reason for hiding this comment

edersonlucas commented Mar 13, 2023

ichelsea commented Mar 13, 2023

broccolinisoup commented Mar 13, 2023

broccolinisoup commented Mar 13, 2023

edersonlucas commented Mar 13, 2023

edersonlucas commented Mar 14, 2023

edersonlucas commented Mar 15, 2023

broccolinisoup left a comment

Choose a reason for hiding this comment

edersonlucas commented Mar 28, 2023

broccolinisoup commented Mar 29, 2023

edersonlucas commented Mar 29, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023 • edited Loading

broccolinisoup commented Mar 30, 2023

broccolinisoup commented Mar 30, 2023

edersonlucas commented Mar 30, 2023

edersonlucas commented Mar 10, 2023 •

edited

Loading

changeset-bot bot commented Mar 10, 2023 •

edited

Loading

edersonlucas commented Mar 30, 2023 •

edited

Loading