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

feat: Popup component #372

Merged
merged 35 commits into from
Mar 3, 2024
Merged

feat: Popup component #372

merged 35 commits into from
Mar 3, 2024

Conversation

marc2332
Copy link
Owner

@marc2332 marc2332 commented Nov 6, 2023

Closes #371
Depends on #268
image

@marc2332 marc2332 added the enhancement 🔥 New feature or request label Nov 6, 2023
@marc2332 marc2332 changed the base branch from main to feat/absolute-positioning November 6, 2023 21:07
Copy link

github-actions bot commented Nov 6, 2023

Benchmark for 32183e0

Click to view benchmark
Test Base PR %
benchmarks/big trees (deep + branches + cached) + invalidated node in the middle 926.7±101.47µs 892.8±107.92µs -3.66%
benchmarks/big trees (deep + cached) + invalidated node in the bottom 87.1±103.37µs 87.1±88.71µs 0.00%
benchmarks/big trees (deep + cached) + invalidated node in the middle 97.4±8.76µs 101.5±88.89µs +4.21%
benchmarks/big trees (deep + cached) + invalidated node in the top 151.2±3.89µs 154.6±1.77µs +2.25%
benchmarks/big trees (deep) nodes=10000, depth=14 200.2±5.43µs 202.1±4.56µs +0.95%
benchmarks/big trees (deep) nodes=100000, depth=17 3.1±0.12ms 2.9±0.10ms -6.45%
benchmarks/big trees (deep) nodes=4000, depth=12 100.9±5.01µs 104.8±78.97µs +3.87%
benchmarks/big trees (wide) nodes=1000, depth=1 150.4±1.08µs 155.3±2.77µs +3.26%
benchmarks/big trees (wide) nodes=10000, depth=1 1563.8±14.40µs 1585.8±22.38µs +1.41%
benchmarks/big trees (wide) nodes=100000, depth=1 29.8±1.68ms 29.8±0.69ms 0.00%

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: Patch coverage is 97.70115% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 71.00%. Comparing base (fda8624) to head (65b62e2).
Report is 2 commits behind head on main.

❗ Current head 65b62e2 differs from pull request most recent head f0e0733. Consider uploading reports for the commit f0e0733 to get more accurate results

Files Patch % Lines
crates/components/src/popup.rs 96.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   70.62%   71.00%   +0.37%     
==========================================
  Files         158      160       +2     
  Lines       15025    15108      +83     
==========================================
+ Hits        10611    10727     +116     
+ Misses       4414     4381      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from feat/absolute-positioning to main November 11, 2023 18:35
@marc2332 marc2332 added this to the 0.2.0 milestone Nov 13, 2023
Copy link

Benchmark for bc47f54

Click to view benchmark
Test Base PR %
benchmarks/size=100001 depth=2 wide=100000 mode=not cached 32.5±1.03ms 31.7±0.90ms -2.46%
benchmarks/size=10001 depth=2 wide=10000 mode=not cached 1577.5±16.06µs 1570.5±17.71µs -0.44%
benchmarks/size=1001 depth=2 wide=1000 mode=not cached 150.3±1.42µs 149.3±1.02µs -0.67%
benchmarks/size=131071 depth=17 wide=2 mode=not cached 27.4±0.33ms 26.2±0.27ms -4.38%
benchmarks/size=16383 depth=14 wide=2 mode=not cached 3.0±0.45ms 2.8±0.56ms -6.67%
benchmarks/size=19531 depth=7 wide=5 mode=cached 600.3±95.90µs 627.8±34.97µs +4.58%
benchmarks/size=19531 depth=7 wide=5 mode=not cached 3.7±0.42ms 4.4±0.18ms +18.92%
benchmarks/size=4095 depth=12 wide=2 mode=not cached 451.1±68.79µs 439.4±58.25µs -2.59%
benchmarks/size=54241 depth=5 wide=15 mode=cached 720.8±86.50µs 680.6±33.52µs -5.58%
benchmarks/size=54241 depth=5 wide=15 mode=not cached 8.1±0.49ms 7.8±0.20ms -3.70%

@marc2332 marc2332 marked this pull request as ready for review January 29, 2024 18:27
@marc2332 marc2332 marked this pull request as draft January 29, 2024 18:27
@marc2332 marc2332 removed the blocked label Feb 6, 2024
Base automatically changed from feat/event-bubbling to main February 6, 2024 21:33
@tigerros
Copy link

How about renaming this to Dialog? That's what it's called in HTML and "popup" is IMO associated with the annoying kind.

@marc2332
Copy link
Owner Author

How about renaming this to Dialog? That's what it's called in HTML and "popup" is IMO associated with the annoying kind.

Doesn't Dialog imply that there must be a dialog to be made? Like, accept, cancel, whatever? Popup seems more generic to me 🤔

@tigerros
Copy link

How about renaming this to Dialog? That's what it's called in HTML and "popup" is IMO associated with the annoying kind.

Doesn't Dialog imply that there must be a dialog to be made? Like, accept, cancel, whatever? Popup seems more generic to me 🤔

Sure, but I just thought that since HTML calls it that, it might be more familiar. Also, it might not always be a "popup", but perhaps a more permanent/complex "subwindow". Like it could have a whole form in it or something, but at that point I wouldn't call it a popup.

@marc2332
Copy link
Owner Author

How about renaming this to Dialog? That's what it's called in HTML and "popup" is IMO associated with the annoying kind.

Doesn't Dialog imply that there must be a dialog to be made? Like, accept, cancel, whatever? Popup seems more generic to me 🤔

Sure, but I just thought that since HTML calls it that, it might be more familiar. Also, it might not always be a "popup", but perhaps a more permanent/complex "subwindow". Like it could have a whole form in it or something, but at that point I wouldn't call it a popup.

What about "Modal"?

@tigerros
Copy link

How about renaming this to Dialog? That's what it's called in HTML and "popup" is IMO associated with the annoying kind.

Doesn't Dialog imply that there must be a dialog to be made? Like, accept, cancel, whatever? Popup seems more generic to me 🤔

Sure, but I just thought that since HTML calls it that, it might be more familiar. Also, it might not always be a "popup", but perhaps a more permanent/complex "subwindow". Like it could have a whole form in it or something, but at that point I wouldn't call it a popup.

What about "Modal"?

Sure, as long as interaction with the rest of the app is disabled.

@marc2332 marc2332 marked this pull request as ready for review February 24, 2024 17:03
@marc2332
Copy link
Owner Author

I'll leave it as "Popup"

@marc2332
Copy link
Owner Author

I should probably support closing popups with Esc

@marc2332
Copy link
Owner Author

marc2332 commented Mar 3, 2024

I should probably support closing popups with Esc

I'll open an issue for this

@marc2332 marc2332 merged commit 27ebbe5 into main Mar 3, 2024
3 of 6 checks passed
@marc2332 marc2332 deleted the feat/popup-component branch March 3, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🔥 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enhancement: Popup / Dialog component
2 participants