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.

Sign up for GitHub

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

Jump to bottom

Make about.html text reflect currently enabled workloads #381

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Make about.html text reflect currently enabled workloads #381

merged 2 commits into from
Feb 27, 2024

Conversation

issackjohn
Copy link
Contributor

@issackjohn issackjohn commented Feb 26, 2024 •
edited
Loading

closes #380

This PR:

  • updates the list in about.html such that it reflects the currently enabled workloads.
  • Adds a missing README.md for Lit complex
  • Adds a missing README.md for javascript-web-components-complex
  • capitalizes all readme files for all the workloads

@issackjohn issackjohn force-pushed the issackjohn/FixWorkloadsListAbouthtml branch from ae0302d to 67ca02d Compare February 26, 2024 20:41
@issackjohn
Copy link
Contributor Author

@rniwa, should I change the readme in lit to be README.md?

@rniwa
Copy link
Member

rniwa commented Feb 26, 2024

@rniwa, should I change the readme in lit to be README.md?

What do you mean? Like capitalize it? I don't think we need to do that.

rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
rniwa
rniwa reviewed Feb 26, 2024
View reviewed changes
about.html Outdated Show resolved Hide resolved
@flashdesignory
Copy link
Contributor

To make it less confusing, can we capitalize all readme files for all the workloads?

@rniwa
Copy link
Member

rniwa commented Feb 26, 2024

To make it less confusing, can we capitalize all readme files for all the workloads?

I'm fine with that. We need to be at least consistent in capitalization between file names & hyperlinks.

@issackjohn
Copy link
Contributor Author

issackjohn commented Feb 26, 2024 •
edited
Loading

To make it less confusing, can we capitalize all readme files for all the workloads?

I'm fine with that. We need to be at least consistent in capitalization between file names & hyperlinks.

Looks like the README files are being treated as newly added files instead? All I did was rename them.

@issackjohn issackjohn marked this pull request as ready for review February 26, 2024 23:05
@flashdesignory
Copy link
Contributor

@issackjohn - did you moved the file from [some path]/readme.md to [some path]/README.md?
Currently I see both (lowercase and uppercase) in the directories.

@issackjohn
Copy link
Contributor Author

issackjohn commented Feb 26, 2024 •
edited
Loading

@issackjohn - did you moved the file from [some path]/readme.md to [some path]/README.md? Currently I see both (lowercase and uppercase) in the directories.

@flashdesignory Oh that's strange. That's not what I did, I renamed the files, but my git wasn't picking up the changes so I ran git config core.ignorecase false. Should I have done the move instead? Are you seeing the duplicates locally or here on the 'Files changed' tab in the PR?

@issackjohn
Copy link
Contributor Author

I think I should revert the commit.

@flashdesignory
Copy link
Contributor

@issackjohn - usually if you need to change casing, it's easiest to move the files to avoid problems.
I'd suggest to revert and rename each readme file.

@issackjohn issackjohn marked this pull request as draft February 27, 2024 00:00
@issackjohn
Copy link
Contributor Author

issackjohn commented Feb 27, 2024 •
edited
Loading

@issackjohn - usually if you need to change casing, it's easiest to move the files to avoid problems. I'd suggest to revert and rename each readme file.

@flashdesignory I tried doing mv [some path]/readme.md [some path]/README.md but it seems like git won't pick up on the change unless i git config core.ignorecase false, but even then, it's still labeling it as a new file like before.

@issackjohn
Copy link
Contributor Author

issackjohn commented Feb 27, 2024 •
edited
Loading

Okay, looks like readme.md -> abc.md ->README.md catches the rename properly.

@rniwa
Copy link
Member

rniwa commented Feb 27, 2024

Could you squash the commits into one?

@issackjohn issackjohn force-pushed the issackjohn/FixWorkloadsListAbouthtml branch from 398ffad to ca21e4f Compare February 27, 2024 00:37
@issackjohn issackjohn marked this pull request as ready for review February 27, 2024 00:38
@rniwa rniwa added the trivial change A change that doesn't affect benchmark results label Feb 27, 2024
julienw
julienw approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
This looks good to me, but we might want to link to the readme file on github. See my comment below for more details.
Not a blocker for me though.

resources/todomvc/architecture-examples/lit-complex/README.md Outdated Show resolved Hide resolved
about.html
<a href="resources/todomvc/architecture-examples/vue/readme.md" target="_blank">TodoMVC-Vue</a>, <a href="resources/todomvc/architecture-examples/jquery/readme.md" target="_blank">TodoMVC-jQuery</a>,
<a href="resources/todomvc/architecture-examples/preact/readme.md" target="_blank">TodoMVC-Preact</a>, <a href="resources/todomvc/architecture-examples/svelte/readme.md" target="_blank">TodoMVC-Svelte</a>,
<a href="resources/todomvc/architecture-examples/lit/readme.md" target="_blank">TodoMVC-Lit</a>
Workloads: <a href="resources/todomvc/vanilla-examples/javascript-es5/README.md" target="_blank">TodoMVC-JavaScript-ES5</a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do y'all think of targeting the README file as hosted on github, so that it can be seen rendered?

Here is how it looks like on browserbench: https://browserbench.org/Speedometer3.0-preview/resources/todomvc/architecture-examples/preact/readme.md

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, linking to Github makes sense to me.

about.html Show resolved Hide resolved
@issackjohn issackjohn merged commit 82e0a46 into WebKit:main Feb 27, 2024
4 checks passed
@issackjohn issackjohn deleted the issackjohn/FixWorkloadsListAbouthtml branch February 27, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rniwa rniwa rniwa approved these changes

@julienw julienw julienw approved these changes

@flashdesignory flashdesignory Awaiting requested review from flashdesignory

@bgrins bgrins Awaiting requested review from bgrins

Assignees
No one assigned
Labels
trivial change A change that doesn't affect benchmark results v3-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The set of TodoMVC workloads listed in about.html needs to be updated.
4 participants
@issackjohn @rniwa @flashdesignory @julienw