-
Notifications
You must be signed in to change notification settings - Fork 374
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
feat: add UI components #781
Conversation
A topic that can be discussed about the current implementation is about components nesting. Right now, components nesting require using plain HTML / class, by using the same component class used by the marked.js component. So, only the way used to call the component ( |
Thank you @alexiscolin, it looks promising. @tbruyelle do you have feedback on the #527? I’m looking for feedback to finish a v1. |
@moul, @tbruyelle I've added all the first components (from the list batch). The code is not perfect yet as I must finish the style and refactor the JS to improve it by factorizing, removing duplicate code and adding a better OOP system across components (so no need to check that yet). But here are the first elements that come to life and with them, some updates regarding the syntax I would like to propose: :::alert (warning)
Warning content
:::alert/
Right now all the components are in the homepage (of course it's temporary, I will move them into |
|
This is neat! This will really improve the writing of contract rendering page. |
If it's pertinent, I think I could create another PR to improve the style/class nomenclature in order to let people customize the design with their own, by overriding ours. Just like in Docusaurus theme for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the direction of this PR, and I can already see how it's going to supercharge our render workflow 🚀
Amazing work, as always 👏
@alexiscolin you've some comments to address before we can merge; can you give a look and fix them or tell us what you suggest instead? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
==========================================
+ Coverage 47.84% 47.85% +0.01%
==========================================
Files 369 369
Lines 62764 63459 +695
==========================================
+ Hits 30028 30371 +343
- Misses 30308 30633 +325
- Partials 2428 2455 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@moul Here is an improved version of this PR. Ready to be reviewed. |
It looks good. ‘could you make sure to give an example not only with the « ::: » lazy replacements but also for someone they would just use plain markdown and html div? ‘I like the idea of extending markdown in an opinionated way; but dislike the idea of making it mandatory to learn. |
❌ Deploy Preview for gno-docs2 failed.
|
Hey! I've had a look at this. I must thank you, @alexiscolin, for bringing more UI components into our rendering, as it is most definitely needed :) I'm not sure, however, that I like this solution of extending the markdown syntax with
My 2c:
A couple ideas for how I could see, for instance, syntax extensions that render the elements you created:
breadcrumb:
This may not be the best, but it is roughly how I think we should go about this, in order to make sure that plain text works too. |
@moul @tbruyelle @zivkovicmilos @thehowl Instead of using html/css and markdown components, I would introduce native It would look like that: <accordion title="Accordion title">
Accordion content
</accordion> Instead of that: <button class="gno-btn gno-accordion-trigger" aria-expanded="true" type="button" id="gno-accordion-[UNIQUE-ID]">Accordion title</button>
<div class="gno-accordion-panel" role="region">
Accordion content
</div> Here is the improvements I see in this update:
|
@alexiscolin I agree with all your points, what about browser compatibility ? The last time I read about web-components, some javascript library client had to be used to make them work properly. |
@tbruyelle Yes I also thought about the browsers compatibility because I had the same feeling. It used to be an issue but now (except for some specific use cases - Cross-root Aria Delegation for example), web-components seems widely supported. |
@alexiscolin That's nice! Though I wonder how web-components fit in with the no-JS compatibility required by gno pages (if that requirement is still relevant). |
@tbruyelle What is that? The whole markdown system relies on JS, even |
@alexiscolin oh OK, I though this was server-side rendered! |
So, we shouldn't have This also means, for instance, that it's still possible to write a native rendering of a Gno's realm Render function (ie. on mobile), without having to I'm in favour of this approach, as it also means we're not adding any special properties to the markdown/commonmark spec. 👍 🎉 😄 |
I'm currently creating a new PR with dedicated web-components |
Markdown UI components
Starting implementation of #439
Implementation of markdown UI components:
Implementation
Here is the first simple component:
Jumbotron
(eg bellow).The implementation come from a
marked.js
extension (in order to avoid useless libs and reuse what is already used), coupled with CSS classes to frame and design it. The idea is to be able to call a component directly from the markdown by using a dedicated convention for components:*Note: using this convention by naming the component would help understand we are manipulating extended markdown UI and quickly use the one we want.
The idea is to iterate and start a conversation over this implementation while I try more complex components such as
inputs
orBreadcrumb
.The result
The first component can be seen at
/r/demo/markdown_test
page.Javascript note
I had to update the main JS function in order to remove the double rendering call from template. It seems the function was created twice (once for
#home
and another for#realms
). I've merged them.Related with #903