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

Move to smaller CSS framework w/ style for public #123

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Conversation

mkly
Copy link
Member

@mkly mkly commented Feb 27, 2024

  • Move to smaller CSS framework with style for public site
  • Use Pico for CSS framework[1]
  • Update styles defensively for placement in public site
  • Basic responsive styling
  • Small template shuffling
  • Remove header and footer as no londer needed

This commit sets the stage to allow for the HTML reports to be either viewed locally or served on the public mlcommons site

[1] Pico was chosen based on the following contraints provided

  1. Pico makes use of derived styles from CSS variables. As we cannot make use of any CSS preprossessing, this allows us to not have know about and style every edge case for colors and spacing
  2. Has pure CSS tooltip as users will be viewing as a local HTML file which will have issues with remote JavaScript
  3. Is small enough to be compressed into a data uri to allow for use to individual html files to be shared(as opposed to directory) and still retain the styles
  4. Pico comes with a procompiled variant that prefixes all styles with .pico which is needed to work within a remote site
  5. Pico has a compatible license (MIT)
  6. Has reasonable usage and remains under active development
  7. It feature-rich enough to require a minimum amount of bespoke styling

Copy link

github-actions bot commented Feb 27, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

* Move to smaller CSS framework with style for public site
* Use Pico for CSS framework[1]
* Update styles defensively for placement in public site
* Basic responsive styling
* Small template shuffling
* Remove header and footer as no londer needed

This commit sets the stage to allow for the HTML reports to be either
viewed locally or served on the public mlcommons site

[1] Pico was chosen based on the following contraints provided

1. Pico makes use of derived styles from CSS variables. As we cannot
   make use of any CSS preprossessing, this allows use to not have no
   about and style every edge case for colors and spacing
2. Has pure CSS tooltip as users will be viewing as a local HTML file
   which will have issues with remote JavaScript
3. Is small enough to be compressed into a data uri to allow for use to
   individual html files to be shared(as opposed to directory) and still
   retain the styles
4. Pico comes with a procompiled variant that prefixes all styles with
   `.pico` which is needed to work within a remote site
5. Pico has a compatible license (MIT)
6. Has reasonable usage and remains under active development
7. It feature-rich enough to require a minimum amount of bespoke styling
Copy link
Collaborator

@dhosterman dhosterman left a comment

Choose a reason for hiding this comment

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

This generally seems fine! A couple of things:

  1. Even though you have data-theme="light" specified in the container, the page is adopting my Mac's dark mode, which makes it very much not match with our designs;
  2. index.html has no content and should probably contain a least a rudimentary link to get to benchmarks.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Thank you for the feedback. I was not aware the index.html had content previously. Is there a link you have that shows that is currently there? Unfortunately, the only thing I've seen in my builds of coffee was a blank page so I feel like maybe I was building something different. At the time I assumed this had lined up with the fact that there isn't technically ever going to be a landing page that we control with this. Would it just make sense to remove index.html altogether since we aren't going to actually be serving that from anywhere?

@dhosterman
Copy link
Collaborator

Yeah, that's totally fine too!

Index.html had no actual content, but did contain the nav header and footer. The nav header had a link to the benchmarks. So it was possible to go from index.html to the benchmarks whereas now it is not.

I think if I'm building something like this and looking at it locally, the first thing I'm looking for is an index.html file. It feels to me like that is the right approach for local HTML report generation, even though I agree with you that it's not useful for deploying it to a MLC site or whatever.

@wpietri
Copy link
Contributor

wpietri commented Feb 28, 2024

I think there are two use cases for index.html. One is when people run the reports locally and want to view them. We don't want them to have to rummage through a big list of files and guess as to what to see. The other is when we upload them to a preview website, as we do currently with Google Cloud. People going to the root should see something useful.

I agree the current one is a little weird, as you have to know the to click on the "Benchmarks" menu at the top, so perhaps we could make it more obvious here.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Just to make sure I fully cover all dark mode scenarios possible, could you give me your browser, OS, and how you have dark mode enabled. For example if on a mac, most people have it enabled via the mac ui display settings. Also, if you could just provide a super quick screenshot it would help me to make sure I'm looking at the right scenario. Thank you

@dhosterman
Copy link
Collaborator

Just to make sure I fully cover all dark mode scenarios possible, could you give me your browser, OS, and how you have dark mode enabled. For example if on a mac, most people have it enabled via the mac ui display settings. Also, if you could just provide a super quick screenshot it would help me to make sure I'm looking at the right scenario. Thank you

Of course!

I'm using Arc Browser 1.31.2 on MacOS 14.2.1. Dark mode is enabled via the Mac UI Display Settings. Here's what it looks like:

CleanShot 2024-02-28 at 12 24 18

@dhosterman
Copy link
Collaborator

And for contrast, here's what it looks like when I switch to light mode. I've blinded myself for science. You're welcome.
CleanShot 2024-02-28 at 12 25 26

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Thanks so much for those screenshots and info. Super helpful. Can you do a quick check with Google Chrome and see if you see the same thing in Dark Mode? I just want to rule out that it is browser specific.

@dhosterman
Copy link
Collaborator

Yes, it's the same with Chrome -- I should have mentioned that Arc is Chromium-based.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Thanks everyone for all the feedback for index.html. It feels like we've expanded the scope outside of what this original PR was intended for. This was intended at the absolute least minimum amount of changes to swap out the frameworks. Let's circle back and create a new issue for what we can do with an index.html

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

data-theme="light" is now added to <html> which is actually what the docs stated. A quick test on Linux and Mac Chrome both confirms what you saw and now seems to indicate adding to <html> is working. Thank you for spotting that.

@mkly mkly requested a review from dhosterman February 28, 2024 19:57
Copy link
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

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

Just tried looking at the output and the HTML looks fine to me, so as soon as we fix the index page regression, I think this is good to go.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Just tried looking at the output and the HTML looks fine to me, so as soon as we fix the index page regression, I think this is good to go.

Thank you. Could you help me to understand a bit more about what has regressed from the initial index.html? I'll be happy to add that in for you.

@wpietri
Copy link
Contributor

wpietri commented Feb 28, 2024

As mentioned above, there are two use cases for the index.html file, and this breaks both of them.

Contrast this: http://35.241.60.176/

with the current output. In the previous, I can click on "Benchmarks" to get to the benchmarks. In the new one, I get a blank page.

I'd be fine with something that makes the new index.html more useful, but it at least shouldn't be worse.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

Thanks for the link and additional info. It is my understanding that is showing a header and a footer that no longer exists as that was a placeholder for embedding into a site. I'm happy to implement anything you'd feel good about here. How about I provide a few quick options to you to decide from to help move this forward? Please feel free to suggest something else as well.

  1. Move benchmarks.html to index.html as index.html will never be included in the live main site
  2. Add a link to bookmarks.html from index.html

Thanks again.

@dhosterman
Copy link
Collaborator

My inclination here is to throw a really simple link on index.html to benchmarks.html and we can update index.html later on to be a local-only landing page if we want to.

@mkly
Copy link
Member Author

mkly commented Feb 28, 2024

My inclination here is to throw a really simple link on index.html to benchmarks.html and we can update index.html later on to be a local-only landing page if we want to.

Great idea. See following screenshot for update rendering.
image

@dhosterman
Copy link
Collaborator

Looks great!

@wpietri
Copy link
Contributor

wpietri commented Feb 28, 2024

I'm all for it too.

@mkly mkly requested a review from wpietri February 28, 2024 22:26
Copy link
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

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

Fab. Thanks!

Copy link
Collaborator

@dhosterman dhosterman left a comment

Choose a reason for hiding this comment

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

Great! Thanks!

@mkly mkly merged commit a97ddff into main Feb 28, 2024
2 checks passed
@mkly mkly deleted the mkly/website-stuff-1 branch February 28, 2024 22:29
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants