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

TodoMVC Angular Bug Fixes #312

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

flashdesignory
Copy link
Contributor

this slipped through somehow, but the dynamic classes of the todo items wasn't applied correctly.
Additionally, I needed to override some css styles to ensure the custom component don't interfere with display.

@kara

Copy link

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lpardosixtosMs lpardosixtosMs left a comment

Choose a reason for hiding this comment

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

LGTM

@lpardosixtosMs
Copy link
Contributor

lpardosixtosMs commented Sep 21, 2023

@flashdesignory Would you mind rebuilding the angular complex version too?
@issackjohn maybe we should add some scripts to keep the standalone versions and their complex counterparts in sync.

@flashdesignory
Copy link
Contributor Author

@flashdesignory Would you mind rebuilding the angular complex version too? @issackjohn we should add some scripts to keep the standalone versions and their complex counterparts in sync.

I just have to run npm install on the big dom generator and npm run build in the complex version, correct?

@issackjohn
Copy link
Contributor

@flashdesignory Would you mind rebuilding the angular complex version too? @issackjohn we should add some scripts to keep the standalone versions and their complex counterparts in sync.

I just have to run npm install on the big dom generator and npm run build in the complex version, correct?

I also run npm install in the complex directory. I'm adding these prerequisites to a build script in the complex version in a follow-up PR which was promised.

@flashdesignory
Copy link
Contributor Author

@flashdesignory Would you mind rebuilding the angular complex version too? @issackjohn we should add some scripts to keep the standalone versions and their complex counterparts in sync.

I just have to run npm install on the big dom generator and npm run build in the complex version, correct?

I also run npm install in the complex directory. I'm adding these prerequisites to a build script in the complex version in a follow-up PR which was promised.

yup, did that as well. should be all in this pr now. Please let me know if I forgot anything.

@bgrins bgrins requested review from julienw and removed request for bgrins September 22, 2023 00:01
@rniwa
Copy link
Member

rniwa commented Sep 22, 2023

Do we have before/after numbers on browsers?

@flashdesignory
Copy link
Contributor Author

Do we have before/after numbers on browsers?

The css bug fixes address user interaction, but don't change the test steps behavior.
We don't edit the todo items in the suites and test that behavior.

@flashdesignory
Copy link
Contributor Author

chrome before:
chrome_before
chrome after:
chrome_after

safari before:
safari_before
safari after:
safari_after

firefox before:
firefox_before
firefox after:
firefox_after

@flashdesignory flashdesignory merged commit 81c835f into WebKit:main Sep 25, 2023
4 checks passed
@flashdesignory flashdesignory deleted the angular-fix branch September 25, 2023 16:10
Comment on lines +31 to +33
app-todo-item:last-child {
border-bottom: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, :last-child can be costly, and this might explain why all browsers see a slight increase in their runtime. But I don't mind keeping it as it's very likely that this is used this way in real-world websites.

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.

Sorry for the delay, this looks good to me.

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.

6 participants