eclipse-ee4j / starter

Eclipse Starter for Jakarta EE
Eclipse Public License 2.0
50 stars 39 forks source link

Header must be fetched from jakarta.ee #246

Open ivargrimstad opened 1 year ago

ivargrimstad commented 1 year ago

The header (with menu), as well as the footer, must be synced with the one at https://jakarta.ee

This is the Starter header today:

starter-INCORRECT

This is how it should be:

jakarta ee-CORRECT

The sources for the https://jakarta.ee website can be found here: https://github.com/jakartaee/jakarta.ee It is built with Hugo, but should it should be possible to include the header and footer in Faces.

yashTEF commented 4 months ago

Yeah, I have opened a PR: https://github.com/jakartaee/jakarta.ee/pull/1925 which needs to be merged.

Once that is done I think the outstanding PR will be ready for review

autumnfound commented 4 months ago

Yeah, I have opened a PR: jakartaee/jakarta.ee#1925 which needs to be merged.

Once that is done I think the outstanding PR will be ready for review

We've now merged that change, I hadn't seen the email for the updated review. Should be live in the next 5 minutes!

yashTEF commented 3 months ago

Hi @autumnfound , I noticed when fetching the footer from the Jakarta.ee endpoint the html fragment that I am receiving in response does not have the opening tags as well as the html code for some of the elements for e.g. html, body tags don't have an opening tag.

For inspecting I tried a curl request and viewed the response, Is there any particular reason for this?

m-reza-rahman commented 3 months ago

For Faces in particular, valid XHTML would be far preferable.

autumnfound commented 3 months ago

For inspecting I tried a curl request and viewed the response, Is there any particular reason for this?

When it was generated, this was stated as the header and footer of the page that come together to create the base Jakarta page. When combined, the final output would be valid HTML. The intention is that it could be easier to do the string replaces for the placeholders in the header, and attempts to make it less complicated for integration.

For Faces in particular, valid XHTML would be far preferable.

If it would make it easier, we can look at creating a single HTML document that would be the full page, where the body content would need to be injected into the DOM manually. Should we start tracking a request for that change?

kito99 commented 3 months ago

Another, less "proper" approach would be to just strip out the and sections from the content returned for the footer.

autumnfound commented 3 months ago

Another, less "proper" approach would be to just strip out the and sections from the content returned for the footer.

If you are saying we can include the footer and header sections as valid HTML, this gets really sticky as the content doesn't quite split cleanly, and would likely just introduce more files that would make this even worse to maintain.

EDIT: accidentally double posted some responses, apologies!

kito99 commented 3 months ago

@autumnfound no, I'm saying you leave it as-is, and we strip out the offending end tags on our end.

yashTEF commented 3 months ago

If it would make it easier, we can look at creating a single HTML document that would be the full page, where the body content would need to be injected into the DOM manually. Should we start tracking a request for that change?

If it makes easier I am able to convert HTML fetched from the url and parse them into well formed XHTML during the build itself using maven plugins.

This works well for the header fragment, but for the footer it removes head and body tags as they don't have matching opening tags, if that can be resolved I should be able to convert the footer HTML to valid XHTML like in the case of header.

autumnfound commented 3 months ago

If it makes easier I am able to convert HTML fetched from the url and parse them into well formed XHTML during the build itself using maven plugins.

If we wanted to use the same "stitching" solution where we give parts of the page to stitch into a larger template, we'd likely have to introduce another 2-3 page fragments at least to get this solution closer to fully functional. There would still likely be some sections where you'd have to hardcode some HTML which would lead to more brittle code, as it would rely on nothing in our styling/page structure changing. Relying on a static structure isn't necessarily the safest thing as it may lead to sudden breaks since you aren't using a static version of the site.

This works well for the header fragment, but for the footer it removes head and body tags as they don't have matching opening tags, if that can be resolved I should be able to convert the footer HTML to valid XHTML like in the case of header.

I've created a PR to add the "full page" template to our base theme. This will need to be reviewed, released, and updated on the Jakarta site, but I believe this will likely be a closer solution to what you need. The change here is that you'll need to update the document model to inject the content into the page. The current query to target will be div#content-placeholder, and you can see the base theme version introduced by the PR on this page: https://preview-371--webdev-eclipse-org-docs-hugo.eclipsecontent.org/docs/hugo/templates/page/

autumnfound commented 3 months ago

If the above is what you need, let me know and I can start the process of getting this page on the Jakarta EE site. If it doesn't we'll need to figure out something else, as I'm not familiar with Faces as I work with a framework that uses a different templating engine which has it's own quirks

autumnfound commented 2 months ago

@yashTEF bumping thread, does the above work, or should we look into something else?

yashTEF commented 2 months ago

@yashTEF bumping thread, does the above work, or should we look into something else?

I am trying to understand the change, how can I use the above to fix the problem?

From what I can understand there would be a single page template instead of header/footer fragments and the page content will be added by manipulating DOM.

@kito99 , does this help or we'll have to change the approach a bit from our side?

autumnfound commented 2 months ago

I am trying to understand the change, how can I use the above to fix the problem?

From what I can understand there would be a single page template instead of header/footer fragments and the page content will be added by manipulating DOM.

Yep, that's correct! This would eliminate the problem where you have incomplete xhtml fragments which led to some truly strange behaviour where nodes would generate opening tags and lead to bad injection.

The only other solution I can think of would have something along the lines of the following (which would require a different change), and isn't ideal as it would lead to issues if we ever changed the HTML around the main content, and would still require DOM manipulation to fix the title in the metadata of the page in the header section (excuse any pseudo code, I don't use xhtml haha):

<html>
  <xhtml:import uri=".../templates/header" />
  <body>
    <xhtml:import uri=".../templates/content-preamble" />
    <main>
      <div class="container padding-bottom-30">
         <div class="row">
           <div class="col-md-18">
             <div class="default-breadcrumbs hidden-print" id="breadcrumb">
               <div>
                 <div class="row">
                   <div class="col-sm-24">
                     <ol aria-label="Breadcrumb" class="breadcrumb">
                       <li><a href="...">Home</a></li>
                       ... (contextual breadcrumbs here)
                       <li class="active" aria-current="page"><a href="...">{{TITLE}}</a></li>
                     </ol>
                   </div>
                 </div>
               </div>
             </div>
             <div id="content-placeholder">

             ...

             </div>
           </div>
         </div>
       </div>
     </main>
    <xhtml:import uri=".../templates/footer" />
  </body>
</html>
autumnfound commented 2 months ago

@yashTEF @kito99 if the mixed implementation I wrote up in pseudo code works better than the other 2 proposed, let me know so I can implement the needed fragments! I'm just waiting on feedback to implement and merge at this point.