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

fix: Enforce minimum width for CheckboxField #712

Merged
merged 6 commits into from
Nov 16, 2022

Conversation

engfragui
Copy link
Contributor

@engfragui engfragui commented Nov 14, 2022

Short description

While polishing our AI Task Creator UI Extension implementation (specs, GitHub epic, Twist thread), we realised that the checkboxes would be smaller than expected when text was longer than average:

05b6-5767-bb71-91606eaf090c

9221-e64a-cc4a-cbc615de3c54

This is due by the fact that reactist's CheckboxField does not enforce a minimum width for the correspondent checkbox:

CleanShot 2022-11-14 at 12 38 20

Closeup view:

CleanShot 2022-11-14 at 12 53 18

(source: reactist storybook)

This PR enforces a minimum width (24 pixels) for the CheckboxField field:

CleanShot 2022-11-14 at 14 07 32

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

Even though I don't think anyone is banking on this "indefinitely shrinking" feature, this is technically a breaking change, as the checkbox corresponding to the CheckboxField element won't keep shrinking indefinitely, but will now have a minimum width.

Checklist for reviewer

  • Look at code change in src/new-components/checkbox-field/checkbox-icon.tsx
  • Verify that all the rest of the automated changed brought in by npm run build-all makes sense (they seem like a lot of changes, that's why I'm mentioning it)
  • Feel free to test these changes by adding a very long text into your local storybook (localhost:6006) (example)

@engfragui engfragui marked this pull request as ready for review November 14, 2022 13:38
@engfragui engfragui requested review from a team and nats12 and removed request for a team November 14, 2022 13:38
@engfragui engfragui self-assigned this Nov 14, 2022
Copy link
Contributor

@nats12 nats12 left a comment

Choose a reason for hiding this comment

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

I'm not sure about the vendor files, I can't see those in the last few PRs created. CC'ing @gnapse who has recently worked closely with Reactist to double check. Are these vendor files normally generated when building?

src/new-components/checkbox-field/checkbox-icon.tsx Outdated Show resolved Hide resolved
@engfragui
Copy link
Contributor Author

engfragui commented Nov 14, 2022

Re: vendors file

I was also somewhat skeptical, but I'm now looking at older PRs and it seems like they appear in there too (example, example, example) 🤷‍♀️

CHANGELOG.md Outdated Show resolved Hide resolved
@gnapse
Copy link
Contributor

gnapse commented Nov 14, 2022

Regarding the vendors file, this is something we want to get rid, and not have to do (see #675), but for now we have to do it. I think this is what updates the static storybooks build hosted in https://doist.github.io/reactist

@gnapse
Copy link
Contributor

gnapse commented Nov 15, 2022

@engfragui after you merge this, you need to create a new release:

  1. Visit https://github.com/Doist/reactist/releases/new

  2. Do as instructed in the video below

    Video
    CleanShot.2022-11-15.at.12.18.29.mp4
  3. Check in the actions tab that this triggered an action to publish to npm

  4. Once the action is done, you can check https://npmjs.com/package/@doist/reactist to see that you have a new public release.

Let me know if you need more help.

This is also something that we should probably document elsewhere. Or better yet, to fully automate it, so that merely merging something would trigger the whole process.

@engfragui engfragui merged commit 3166d01 into main Nov 16, 2022
@engfragui engfragui deleted the francesca/checkbox-min-width branch November 16, 2022 15:44
@engfragui
Copy link
Contributor Author

@gnapse Everything is pretty clear in the README's releasing section as well so I would say it's already documented.

However:

  • I notice that the reminder to run npm run build-all appears only in the PR template and not in the actual README so maybe we can add that to the README as well
  • I like your numbered list better than the format I see in the README's releasing section

If my notes make sense, I will send a quick PR with these updates.

@engfragui
Copy link
Contributor Author

engfragui commented Nov 16, 2022

@gnapse Errr does https://github.com/Doist/reactist/actions/runs/3480872733 look good? It does to me (no errors).

I do see the new version in https://github.com/Doist/reactist/releases/tag/v17.0.1, but not in https://www.npmjs.com/package/@doist/reactist

Let me know if you spot something that looks fishy. Thank you

For now, I'll hold off before merging the correspondent todoist-web PR (https://github.com/Doist/todoist-web/pull/5388)

@scottlovegrove
Copy link
Contributor

@engfragui
Copy link
Contributor Author

Err ok, https://www.npmjs.com/package/@doist/reactist looks like this for me

Screenshot 2022-11-16 at 17 46 10

Screenshot 2022-11-16 at 17 47 15

I guess cached/delayed. Thank you Scott

@scottlovegrove
Copy link
Contributor

Almost certainly cached, because I have
image

@gnapse
Copy link
Contributor

gnapse commented Nov 16, 2022

If my notes make sense, I will send a quick PR with these updates.

Sure @engfragui, send a PR to change the README to make it better. Whatever you think, as a newcomer to this repo, that should be improved, most probably needs improvement. We the usual suspects contributing here may have gotten used to the status quo too much.

Thanks for your contributions, BTW.

@engfragui
Copy link
Contributor Author

No action needed from you, but here's the quick README update I mentioned I wanted to take care of: #726

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.

None yet

4 participants