Skip to content

Performance: Use Identifier helper in more cases#9558

Merged
ScarletKuro merged 2 commits intoMudBlazor:devfrom
xC0dex:refactor/use-identifier
Aug 5, 2024
Merged

Performance: Use Identifier helper in more cases#9558
ScarletKuro merged 2 commits intoMudBlazor:devfrom
xC0dex:refactor/use-identifier

Conversation

@xC0dex
Copy link
Member

@xC0dex xC0dex commented Aug 2, 2024

Description

  • Use Identifier helper class where it's possible.
  • Removed unused internal uid field in Column component.

How Has This Been Tested?

Existing tests.

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.

@codecov
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.56%. Comparing base (28bc599) to head (df50831).
Report is 395 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9558      +/-   ##
==========================================
+ Coverage   89.82%   90.56%   +0.73%     
==========================================
  Files         412      406       -6     
  Lines       11878    12725     +847     
  Branches     2364     2465     +101     
==========================================
+ Hits        10670    11525     +855     
+ Misses        681      639      -42     
- Partials      527      561      +34     

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

@xC0dex xC0dex marked this pull request as ready for review August 2, 2024 15:09
@xC0dex xC0dex added performance Time/memory/CPU/allocation performance characteristics PR: needs review labels Aug 2, 2024
@xC0dex xC0dex requested review from ScarletKuro and henon August 2, 2024 15:10
@dennisrahmen
Copy link
Contributor

dennisrahmen commented Aug 2, 2024

I would think that the uid was probably implemented because of the last checkbox in this issue: #6555

It was just never finished.

@ScarletKuro
Copy link
Member

Just a quick question, on why did we remove the prefixes like mudnavmenu or snackbar

@xC0dex
Copy link
Member Author

xC0dex commented Aug 5, 2024

Just a quick question, on why did we remove the prefixes like mudnavmenu or snackbar

For me, this is only a generated ID and there was no benefit for me keeping the prefix. In addition, this was inconsistently used, so I guess this was only used to prevent invalid characters as the first letter.

@ScarletKuro
Copy link
Member

Just a quick question, on why did we remove the prefixes like mudnavmenu or snackbar

For me, this is only a generated ID and there was no benefit for me keeping the prefix. In addition, this was inconsistently used, so I guess this was only used to prevent invalid characters as the first letter.

Ok, I'm just hoping that nobody used this for Selenium testing etc.

@ScarletKuro ScarletKuro merged commit 97c83ab into MudBlazor:dev Aug 5, 2024
@xC0dex
Copy link
Member Author

xC0dex commented Aug 5, 2024

Ok, I'm just hoping that nobody used this for Selenium testing etc.

If something occurs, please mention me. Generated IDs shouldn't be used as a selector.
I was also thinking about supporting data-testid in MudBlazor. E.g., Playwright has a dedicated method for this. But this would require some configuration. I, personally, don't want such IDs in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Time/memory/CPU/allocation performance characteristics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants