Add new props to the Hr element

[khushal87]

Currently, the Hr component has only three props as variant, size, and color. We can have some more useful props such as inset(left, right, middle), orientation(vertical, horizontal), subHeader, and width.

Note: I am considering Hr in this package to be a standard Divider

Progress:

• Inset
• Orientation
• subHeader
• subHeaderStyle
• Docs
• Tests
• Stories

[khushal87]

Hello admins, I want to start working on this great repository! I want to start working on this issue if it seems relevant to you all. Thank you. @cstrat @rtivital @srezanova

[rtivital]

Hi @khushal87 sounds good!
Can you describe what subHeader prop will do?
Also please make sure that component works fine with Menu and Card after the changes (in horizontal orientation only), since these components have some context styles for Hr.

[khushal87]

@rtivital Thanks, sure I will check the mentioned. For subheaders, you can refer https://material.io/components/dividers#usage.
Also, can you explain what is the realtime testing method you are using to test the changes in the package components visually, this will improve my workflow.

[rtivital]

Yep, subHeader looks great, let's add it.
To get started locally:

• fork this repo
• clone your fork
• install dependencies with yarn
• run storybook – npm run storybook – we use storybook for local development, add stories that you need to test new features, don't worry on how it looks in storybook – it's not published anywhere
• don't forget to add props descriptions and tests
• after you finish adding new features run npm run docs – it will start local documentation server
• Add docs and demos to Hr component
• Done! Run npm test to check if everything is fine and send a PR

Here is a link to contributing guidelines – https://mantine.dev/pages/contribute/ with more in depth review

[khushal87]

Thanks a lot, @rtivital. I will soon raise a PR for the same.

[rtivital]

@khushal87 sorry, I forgot one thing, when you will create a PR, please request a merge to next-1.1.0 as this feature is planned to release in minor release

[cstrat]

Thank you. @cstrat @rtivital @srezanova

Don't include me in that list, I've just helped fix some typos. Hoping I can contribute more though!

[khushal87]

@rtivital sure! 😄

[khushal87]

Hey, @rtivital I was just running the storybook locally and facing this problem here. Can you help?

ERROR in ./configuration/storybook/generated-stories-entry.js
Module not found: Error: Can't resolve 'C:Usershrithmantinesrc' in 'C:\Users\hrith\mantine\configuration\storybook'        
 @ ./configuration/storybook/generated-stories-entry.js 7:37-127
 @ multi ./node_modules/@storybook/core-server/dist/cjs/globals/polyfills.js ./node_modules/@storybook/core-server/dist/cjs/globals/globals.js ./configuration/storybook/storybook-init-framework-entry.js ./node_modules/@storybook/addon-docs/dist/esm/frameworks/common/config.js-generated-other-entry.js ./node_modules/@storybook/addon-docs/dist/esm/frameworks/react/config.js-generated-other-entry.js ./node_modules/@storybook/addon-actions/dist/esm/preset/addDecorator.js-generated-other-entry.js ./node_modules/@storybook/addon-actions/dist/esm/preset/addArgs.js-generated-other-entry.js ./node_modules/@storybook/addon-backgrounds/dist/esm/preset/addDecorator.js-generated-other-entry.js ./node_modules/@storybook/addon-backgrounds/dist/esm/preset/addParameter.js-generated-other-entry.js ./configuration/storybook/preview.js-generated-config-entry.js ./configuration/storybook/generated-stories-entry.js (webpack)-hot-middleware/client.js?reload=true&quiet=false&noInfo=undefined 

[rtivital]

Let me check, I haven't tried on windows yet

[khushal87]

There's some path issue depending on the operating system according to me.

[rtivital]

Sorry, I've spent half an hour debugging this on windows 10, and nothing seems to work, I can propose only to skip the storybook part and develop straight in docs, it also has HMR and should work on windows

[khushal87]

Okay, i will go through this issue, if I find the solution I will propose a fix in the meantime @rtivital

[rtivital]

Okay, sounds good

[khushal87]

Hey @rtivital I made this working by this change in the ./configuration/storybook/main.js

module.exports = {
  stories: [],
  addons: ['@storybook/addon-essentials'],
  webpackFinal: async (config) => {
    config.resolve = {
      ...config.resolve,
      plugins: [
        ...(config.resolve.plugins || []),
        new TsconfigPathsPlugin({
          extensions: ['.ts', '.tsx', '.js'],
          configFile: path.join(__dirname, '../../tsconfig.json'),
        }),
      ],
    };

    // Turn off docgen plugin as it breaks bundle with displayName
    config.plugins.pop();

    return config;
  },
};

I changed stories to []. Can you check if this works on other OS too, if this will work I will raise a PR and this would probably be a fix. Though, the stories are still loading for a long time after the localhost:7000 triggers. So, this is clear that the issue is with the path of fetching the stories. Now we need to add a variable that will adapt all the OS.

Reference: redwoodjs/redwood#954 (comment)

The loading screen -

[rtivital]

Well, that fixes the problem but also removes all stories 😄 , the value for stories is required for storybook to work

[khushal87]

Exactly, so the next steps are adding a suitable path here to fetch all the stories, let me see if I can fix this too.

[khushal87]

@rtivital [path.resolve(__dirname, '../../src/**/*.story.@(ts|tsx)').replace(/\/g, '/')] this will work fine in every OS in my opinion can I raise a PR for it? 😄

[rtivital]

Yep, just checked, it works on MacOS:

stories: [path.resolve(__dirname, '../../src/**/*.story.@(ts|tsx)').replace(/\//g, '/')],

Please create the PR to master

[khushal87]

Thanks @rtivital

[khushal87]

@rtivital I have a quick doubt. Why is the default Hr variant 'dashed'? I believe the most suitable would be 'solid'

[rtivital]

I just always use dashed variant in my projects and never solid,but now that you add more features, lets change it to solid

[khushal87]

Okay. Thank you.

[khushal87]

@rtivital I believe that width is a better name instead of size for Hr and Dividers. What is your opinion? Should I change it?

[rtivital]

No let's leave size – it does not depend on the context. In vertical orientation size means width in horizontal – height. It's consistent with the rest of API

[khushal87]

Okay, got it. I am facing some issues with the vertical orientation styles, it is not coming up well. Let's see if different approaches work. Apart from that, everything is good to go. 💯

[rtivital]

Resolved in 1.1.0