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

External version banner initial style #18

Merged
merged 18 commits into from
Apr 12, 2023
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 22, 2023

I'm opening this PR here to receive the initial feedback because I'm sure this is not probably the path we want to follow. My CSS skills are hardcoded in the 90's still 😄

Screenshot_2023-03-22_23-33-13

See it in action at: https://readthedocs-client--18.org.readthedocs.build/en/18/pelican/

@humitos
Copy link
Member Author

humitos commented Mar 23, 2023

I updated a little more and I also tested on Pelican.

It now uses self-contained (bundled) fontawesome, without relying on the theme to have them installed.

Peek 2023-03-23 13-32

@humitos humitos requested a review from agjohnson March 23, 2023 12:34
We are not used the compiled version currently. The one served by the
application lives in S3 and we can compile and upload to S3 for now.
@humitos humitos marked this pull request as ready for review March 23, 2023 12:36
@humitos
Copy link
Member Author

humitos commented Mar 23, 2023

@agjohnson I'm marking this PR for review to receive some initial feedback.

@humitos
Copy link
Member Author

humitos commented Mar 23, 2023

New idea using a large icon at the beginning of the banner:

Screenshot_2023-03-23_13-42-30

@agjohnson
Copy link
Contributor

agjohnson commented Mar 23, 2023

I might have some suggestions for playing around with some of the styles and content here. Is it possible to bring up the warning banner using the dev server QA tests?

A few things I might want to try are a narrower banner, perhaps a different icon and header. I don't have any solid ideas just yet though. I think what you have so far is a great start!

@humitos
Copy link
Member Author

humitos commented Mar 23, 2023

Yo can start the development RTD environment as usual and then run "npm run dev" from this project.

Then build "manual-integrarions" branch in test-builds and it should show the new js client.


Another, more simple way, is to just run "npm run dev" and open http://localhost:8000/pelican/

@humitos humitos closed this Mar 23, 2023
@humitos humitos reopened this Mar 23, 2023
@humitos
Copy link
Member Author

humitos commented Mar 23, 2023

Or, even, you can play directly from the build from the PR in Read the Docs 🚀 at https://readthedocs-client--18.org.readthedocs.build/en/18/pelican/

@humitos
Copy link
Member Author

humitos commented Mar 30, 2023

This is what I currently have. I'm staying more conservative with the colors because I don't have those skills 😄 . I'm trying to match the simplicity that we are following in the search UI regarding the amount of colors.

It also follows, more or less, some of the same colors from EthicalAds.

As you can see in the following screenshot, it works nice on Docusaurus too.

Screenshot_2023-03-30_02-00-10

- use a bigger `z-index` to not collide with MkDocs
- add a div that acts as a background shadow
@humitos
Copy link
Member Author

humitos commented Apr 4, 2023

Updates

Peek 2023-04-04 20-04

Peek 2023-04-04 20-00

@agjohnson
Copy link
Contributor

The "warning" bar at the top is nice, but perhaps this could use something other than "warning" as copy? I'd have to think of replacements, but "warning" feels like it's saying "somethings wrong" as opposed to "just so you know".

@humitos
Copy link
Member Author

humitos commented Apr 4, 2023

"Note" (or "Notice" --but I'm not sure if it's correct in this context) may be another option; which does not convey that something is wrong.

@agjohnson
Copy link
Contributor

agjohnson commented Apr 5, 2023

I was thinking more verbose, like a full heading. I put up some changes at #37 but there are a lot. Noted in chat, but this has some illustration of a web component pattern using Lit. This greatly reduced the CSS and the shadow dom isolation makes CSS styling easier.

I'll have some notes here as well, I didn't really get around to that yet. The individual commits have some more digestible changes at least.

I'll see if I can explain some of the smaller changes here next up, but
opening this PR for now

Improvements here:

- Reduced the required CSS/HTML structure in
9867509.
  - Dropped extra unneeded divs
  - Used direct descendant specifiers instead of verbose names
  - Dropped unneeded CSS rules for spacing issues
- In the rest of the commits I explored a Lit web component
implementation more. Improvements from this approach:
- Linking HTML, CSS, and JS is much cleaner, and still uses HTML string
templates even. String templates could be a point of extension later.
- Shadow DOM + CSS is much nicer than trying to inject with arbitrary
HTML/CSS.
- Started to illustrate some patterns we could aim for
  - CSS variable theming
- Addon class abstraction for to standardize enabling/disabling (and
later configuration) of each addon module
  - Notification variants
@agjohnson
Copy link
Contributor

Should be 👍 to merge this and continue I think!

I forgot to mention, I added a style catalog sort of test suite in the dev server at http://localhost:8000/index.html, you can see a bunch of examples there now.

@humitos
Copy link
Member Author

humitos commented Apr 12, 2023

I'm going to merge this and make my changes in different PRs.

@humitos humitos merged commit 23ff104 into main Apr 12, 2023
@humitos humitos deleted the humitos/external-version-banner branch April 12, 2023 09:04
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.

2 participants