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

[DRAFT] Search components integration #2418

Closed

Conversation

FranckyC
Copy link

@FranckyC FranckyC commented Jun 6, 2023

PR Type

Feature

Description of the changes

This is a first non optimized version just to assess the technical feasibility. Not intended to be used as is or integrated elsewhere.

Hi @gavinbarron, @sebastienlevert , as discussed, here is a first functional version of search components integration:

  • Search results
  • Search filters
  • Search verticals

There is still lot of work to do but, at a glance, here are the following changes I've made to make it work:

Tailwind

Added Tailwind CSS integration with postcss and JIT mode. This allow components to use Tailwind styles on demand through dynamically generated tailwind-styles.css file. Just use yarn start as usual and see the content of the file to updated automatically when you add new classes on your components.

This relies on your existing SASS/TS modules transform mechanism. See tailwind task in gulpfile.js. Typically, for a component it means:

import { styles } from './mgt-search-verticals-css';
import { styles as tailwindStyles } from '../../styles/tailwind-styles-css';
...
static get styles() {
  return [
    styles,
    tailwindStyles
  ];
}
...
public render() {
  return html`
      <div class="px-2.5">
        <div class="max-w-7xl ml-auto mr-auto mb-8">
        ...

Dynamic imports

For search filters and performances purpose, I need to load correct locale according to the current language using daysJs library. However, because the current solution is not configured to have multiple bundles, I need to add the inlineDynamicImports flag on the Rollup config to keep my code as is (only on es6 config for now).

As discussed, this is something that needs to be considered in the future to optimize distribution and performances.

Tests

Added test example for search verticals component with @open-wc/testing package, web test runner with esplugin/playwright.
Added also useDefineForClassFields on a custom tsconfig.test.json to make it work with lit (I'm not using Babel).

To run tests and watch mode just use yarn wtr:watch and the VSCode debug configuration "Tests debug (Edge)":

image

Localization

In our company scenario, localization works for both components internal labels but also settings via attributes (ex: vertical name in multiple languages). For setting values, we define the ILocalizedString interface relying on a mandatory "language" property in localization strings:

export const strings = { 
    language: "en-us",
    ....
{
        "key":"tab1",
        "tabName": {
            "default":"Tab 1",
            "fr-fr":"Onglet 1"
        },
        ...

Normally, it is linked to a language provider component globally available on the page and setting this property using the LocalizationHelper but I didn't include it in this demo but you get the idea. Maybe something to think about in the future.

Next steps

Before going forward, we need to decide how to:

  • Integrate these components in MGT. As discussed, it seems to be fairly easy to make them work in the existing MGT stack. However it brings a lot of search dedicated classes not useful for other components, increasing the bundle size for nothing. IMO, this should be either a project on its own in the monorepo structure with its own bundle (like mgt-search, why not with its own stack like Webpack :D) or a separate repo. For the latter, this will probably fall under the microsoft-search organization as the new PnP Modern Search including both components and SPFx webparts).
  • Merge features from MGT existing search components and upcoming ones.
  • Complete integration to adapt with existing MGT features (caching, styles, doc, etc.)

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Jun 6, 2023

Thank you for creating a Pull Request @FranckyC.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@sebastienlevert
Copy link
Contributor

@FranckyC there are a lot of edits on "untouched" files. Can you look into it? We'll be looking into it with the team and evaluate better the next steps! THANKS FOR THE CONTRIBUTION!!!

*/
public static decode(encodedStr: string) {
const domParser = new DOMParser();
const htmlContent: Document = domParser.parseFromString(`<!doctype html><body>${encodedStr}</body>`, 'text/html');

Check warning

Code scanning / CodeQL

Unsafe HTML constructed from library input

This HTML construction which depends on [library input](1) might later allow [cross-site scripting](2).
Comment on lines +8 to +10
return /((([A-Za-z]{3,9}:(?:\/\/)?)(?:[\-;:&=\+\$,\w]+@)?[A-Za-z0-9\.\-]+|(?:www\.|[\-;:&=\+\$,\w]+@)[A-Za-z0-9\.\-]+)((?:\/[\+~%\/\.\w\-_]*)?\??(?:[\-\+=&;%@\.\w_]*)#?(?:[\.\!\/\\\w]*))?)/.test(
url
);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings with many repetitions of '$'.
@FranckyC
Copy link
Author

FranckyC commented Jun 6, 2023

@sebastienlevert this is probably because I merged my forked next/fluentui from my branch that was based on main in the first place so the diff comes from here. Still, the goal is not to merge this one, just for you to test. Once the strategy defined, I'll start from scratch from latest

@gavinbarron gavinbarron deleted the branch microsoftgraph:v3-feature-branch June 27, 2023 16:00
@gavinbarron
Copy link
Member

@FranckyC

I'm sorry, I messed up here, I was cleaning up the old feature branch for v3 and it closed this PR when the target branch was deleted.

Can you please create a new PR targeting main and I'll take a look at how we move forward from here.

@FranckyC
Copy link
Author

FranckyC commented Jul 3, 2023

@gavinbarron done: #2582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants