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

Remove indirection when rendering composer state #5954

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 26, 2024

Stacked on #5953 and #5945


Continues refactoring to prepare for threads.

This is a refactor to remove some intermediate variables that no longer serve us well. We want to read from draft directly so that we can later adjust the conditions to deal with multiple posts.

The last commit moves a grapheme length calculation to the reducer because we won't be able to memoize it in the component anymore (since it will be per-post).

Test Plan

Verify the discard prompt shows up when the post is non-empty (note that just a quote is considered empty).

Verify the grapheme calculations works like before, including with long URLs.

Verify missing ALT detection still works for images and for GIFs.

Verify Post button remains disabled on completely empty posts.

@gaearon gaearon changed the title Thread prepare 2 Remove indirection when rendering composer state Oct 26, 2024
Copy link

Old size New size Diff
7.91 MB 7.91 MB 52 B (0.00%)

@haileyok
Copy link
Contributor

this isn't a new issue, but im wondering why the "require alt text" warning doesn't let you post when adding a gif when there is already alt text set. shouldn't the alt text be getting automatically applied based on the default for those? wouldn't expect the warning to appear in those cases.

@haileyok
Copy link
Contributor

haileyok commented Oct 29, 2024

one other note, and probably not something that is new but didn't check.

  • Add a video and press "Post" before the upload completes
  • Reply button becomes disabled pending the completion of upload
  • The video fails to process/upload. User gets an error
  • User adjusts post content
    • At this point, the reply button never becomes "undisabled" until a new video gets attached
    • As soon as the new video uploads, the post automatically gets posted. There is no warning or "resetting" of the state when the new video gets added to the post

I think a failure should be resetting that "post when finished" state. (For a surefire way to get an upload to finish, can send you a video on Slack that will cause a failure)

Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

lgtm with the two notes above, neither of which i believe are regressions but should get addressed somewhere in this rework

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 29, 2024

this isn't a new issue, but im wondering why the "require alt text" warning doesn't let you post when adding a gif when there is already alt text set. shouldn't the alt text be getting automatically applied based on the default for those? wouldn't expect the warning to appear in those cases.

re: this, we settled on forcing you to open the dialog once to proofread the alt (even though it gets saved from that point on). since sometimes it's very low quality. otherwise the setting would just always pass things through which defeats the purpose of the setting.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 29, 2024

I think a failure should be resetting that "post when finished" state. (For a surefire way to get an upload to finish, can send you a video on Slack that will cause a failure)

yup, confirmed the refactor in #5957 fixed that

Base automatically changed from thread-prepare to main October 29, 2024 20:27
@arcalinea arcalinea temporarily deployed to thread-prepare-2 - social-app PR #5954 October 29, 2024 20:29 — with Render Destroyed
@gaearon gaearon merged commit 96ba645 into main Oct 29, 2024
6 checks passed
@gaearon gaearon deleted the thread-prepare-2 branch October 29, 2024 20:29
estrattonbailey added a commit that referenced this pull request Oct 31, 2024
* origin/main: (213 commits)
  add Thai Language translation support (#5879)
  fix warning on labeler profile: emoji detected but emoji not enabled (#6011)
  Added blur to search's onPressMenu (#6017)
  React compiler beta and reenable rule (#5898)
  Sort imports (#6009)
  Clarify build instructions (#6008)
  Upgrade all tiptap deps to latest (#5232)
  width full on text container (#6007)
  Bump 1.94.0 (#6006)
  Add subtle web hover to interactive rows (#5989)
  Settings revamp (#5745)
  Show almost-instant preview when opening lightbox (#6000)
  Refactor lightbox model to plain object (#5999)
  temp revert to old modal (#6005)
  Extend composer checks to all posts in a thread (#5955)
  Remove indirection when rendering composer state (#5954)
  Refactor composer state for threads (#5945)
  Disable Post button when empty (#5953)
  fix splash screen (#5997)
  Fix video quality for short videos (#5996)
  ...
rshigg pushed a commit to rshigg/social-app that referenced this pull request Nov 2, 2024
* Simplify onPressCancel dismiss condition

* Remove alias variables

* Move grapheme length calculation to reducer
rshigg pushed a commit to rshigg/social-app that referenced this pull request Nov 2, 2024
* Simplify onPressCancel dismiss condition

* Remove alias variables

* Move grapheme length calculation to reducer
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.

3 participants