Skip to content

[OPTIONAL] - updated todomvc styles - #254

Merged
flashdesignory merged 12 commits into
WebKit:mainfrom
flashdesignory:todomvc-styles
Jun 28, 2023
Merged

flashdesignory merged 12 commits into
WebKit:mainfrom
flashdesignory:todomvc-styles

Conversation

@flashdesignory

@flashdesignory flashdesignory commented Jun 22, 2023 •

Copy link
Copy Markdown
Contributor

This pr adds updated styles for todo apps as discussed in the a11y audit: https://docs.google.com/document/d/1oQiN56elDw21EejEY7H7aT6CSJBMDP-9XVjSb6d6B-I/edit

This is also shareable and can be consumed by the lit / web component prs for testing if we want (web component pr basically uses identical styles / structure already).

Folder structure matches the newssite-css setup and the package can be locally installed the same way.

@kara

@bgrins

bgrins commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

Since this shows up as net-new code in GitHub it's hard to differentiate what is part of #112 and what's just part of bringing over the existing CSS. If it's not too much trouble can you either have two separate commits or two separate PRs where first we bring the whole set over unchanged, and then second it applies the a11y improvements?

@flashdesignory

Copy link
Copy Markdown
Contributor Author

That is gonna take some time.. Since the dom structure also changed, even with two prs it'll look pretty messy still.

@flashdesignory

Copy link
Copy Markdown
Contributor Author

What makes it hard to compare against the currently used npm package is that all styles are separated into separate files in this pr (to allow us to build the css modules)

@bgrins

bgrins commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

Since the dom structure also changed

Where did this happen, I don't see it in this PR?

@flashdesignory

Copy link
Copy Markdown
Contributor Author

This pr is just styles, but there are some screenshots in the document mentioned above.

@bgrins

bgrins commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

Clarification after chatting with @flashdesignory - this CSS is targeting the new markup structure https://github.com/WebKit/Speedometer/pull/254/files#diff-c9c4e4297e9dc33905dec9420bb822198a2bd333a48d6231cc23e0df35a5eec7 which currently only the proposed Lit and Web Components workloads use. The idea would be that as other workloads migrate their markup to the new structure they would start using this local package instead of the current CSS.

@flashdesignory

Copy link
Copy Markdown
Contributor Author

Additional clarification: only the web-component version uses the new structure, but it's something that lit could potentially implement to take advantage of the css modules here.

Comment thread .eslintignore Resolved
@julienw

julienw commented Jun 23, 2023

Copy link
Copy Markdown
Contributor

This looks reasonable to me, but I didn't look too closely.

@bgrins

bgrins commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

After some more discussion - my main note here is that it assumes a different DOM structure for the general styling so it's not a straightforward migration for all the workloads to use it - which would be a good outcome compared to having two copies of the CSS and needing to reason about when each one should be used.

So I'd like to double check if we have any blockers or issues to creating a path forward where all workloads are using it (presumably many using just the normal CSS files which are either built into a bundle or using @import). This could be follow up work and IMO can continue into the next half since it's not new workloads or major changes to existing (and AIUI the structure changes are meant as part of the a11y improvements).

One example I thought about is if we introduce a new DOM structure there may be some interdependencies with the complex DOM and codegen work. We confirmed that the refactoring in #246 should make it easy to adapt to a new structure so long as we remember to re-run it.

@flashdesignory

Copy link
Copy Markdown
Contributor Author

No blockers from my side and I'd be down to make the changes. Being able to work on these in the next quarter would be ideal.

@rniwa rniwa added the trivial change A change that doesn't affect benchmark results label Jun 28, 2023
@flashdesignory
flashdesignory merged commit 68c56ff into WebKit:main Jun 28, 2023
@flashdesignory
flashdesignory deleted the todomvc-styles branch June 28, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

trivial change A change that doesn't affect benchmark results

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants