Skip to content

TodoMVC Angular Bug Fixes - #312

Merged
flashdesignory merged 3 commits into
WebKit:mainfrom
flashdesignory:angular-fix
Sep 25, 2023
Merged

TodoMVC Angular Bug Fixes#312
flashdesignory merged 3 commits into
WebKit:mainfrom
flashdesignory:angular-fix

Conversation

@flashdesignory

Copy link
Copy Markdown
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

@kara kara left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@lpardosixtosMs lpardosixtosMs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@lpardosixtosMs

lpardosixtosMs commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

rniwa commented Sep 22, 2023

Copy link
Copy Markdown
Member

Do we have before/after numbers on browsers?

@flashdesignory

Copy link
Copy Markdown
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
Copy Markdown
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
@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
Copy Markdown
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.

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

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