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

Vertical Bar Component #2903

Closed
jbalsas opened this issue Feb 5, 2020 · 20 comments · Fixed by #4892
Closed

Vertical Bar Component #2903

jbalsas opened this issue Feb 5, 2020 · 20 comments · Fixed by #4892
Assignees
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) comp: clay-css Issues related to Clay CSS type: enhancement Issues that are open to bring improvements or refinement of code

Comments

@jbalsas
Copy link
Contributor

jbalsas commented Feb 5, 2020

This is a formal request to implement Application Sidebar as a Clay Component.

We currently lack documentation proper Lexicon definition and documentation for this component, so this task should start by involving @drakonux and @laugardie in analysis and definition tasks. This is already requested in LEXI-806

#### Current Existing Implementations:

PageEditor Sidebar

Screen Shot 2020-02-05 at 12 58 25

Future Implementations:

Product Menu

We currently have a POC for this at https://github.com/eduardoallegrini/liferay-product-menu

Screen Shot 2020-02-05 at 13 59 37

Journal Sidebar

image (3)

/cc @bryceosterhaus, @kresimir-coko, feel free to ping @drakonux for any clarification

@jbalsas jbalsas added rfc Similar to the RFC that are used in React.js, Ember.js, Gatsby.js and Rust, but to mark problems tha 3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) type: enhancement Issues that are open to bring improvements or refinement of code and removed rfc Similar to the RFC that are used in React.js, Ember.js, Gatsby.js and Rust, but to mark problems tha labels Feb 5, 2020
@bryceosterhaus
Copy link
Member

@jbalsas is this already an existing component via clay-css?

@jbalsas
Copy link
Contributor Author

jbalsas commented Feb 5, 2020

I don't think so, but @pat270 might know best. I think @eduardoallegrini and @marcoscv-work can work on this with @pat270 or he can use the linked POC as a base.

@diegonvs diegonvs self-assigned this Feb 5, 2020
@pat270
Copy link
Member

pat270 commented Feb 5, 2020

@jbalsas @bryceosterhaus @marcoscv-work @eduardoallegrini
We will need to add a base component for the skinny toolbar with icons. I'm thinking .tbar-stacked? We will also need to create variations for each, .tbar-product-menu and .tbar-journal-sidebar?

The Product Menu and Journal Sidebar can be Sidebar variations. I think we should name them .sidebar-product-menu and .sidebar-journal.

For the search bar in Product Menu we will need details about focus, disabled and placeholder styles from @drakonux and @laugardie.

Feel free to send a PR otherwise I can get to it after I work on the other milestone 15 stuff.

@pat270 pat270 added the comp: clay-css Issues related to Clay CSS label Feb 5, 2020
@diegonvs
Copy link
Contributor

diegonvs commented Feb 5, 2020

We will also need to create variations for each, .tbar-product-menu and .tbar-journal-sidebar?

I think these sidebar looks the same, only differs by the content. May We could create dark and light variations.

@edalgrin
Copy link

edalgrin commented Feb 6, 2020

Can't we use general names instead of dedicated one? E.g. .tbar-left, .tbar-previous, .tbar-before in this way we can use the same tbars without worry about the misunderstandings the names could create. The same goes for the sidebars .sidebar-left etc..

@jbalsas
Copy link
Contributor Author

jbalsas commented Feb 6, 2020

Yeah, these are not application specific. We're likely to have:

  • Color schemes: Dark/Light
  • Positioning: Left/Right

But they can be used in different apps other than journal and product menu, so we should make them as generic as possible

@edalgrin edalgrin assigned edalgrin and unassigned diegonvs Feb 6, 2020
@edalgrin
Copy link

edalgrin commented Feb 6, 2020

I'm going to work on it

@pat270
Copy link
Member

pat270 commented Feb 6, 2020

Going forward, I'm trying to address some of the concerns expressed in #1104. We should refrain from being too generic because modifying sidebar-dark would cause changes on admin related UI. It seems that is an unwanted behavior for a lot of folks. We really should free up the generic namespaces for devs and scope our stuff to something a little more specific. The component name can be generic relative to Liferay Portal Admin in my opinion.

@diegonvs
Copy link
Contributor

diegonvs commented Feb 6, 2020

Hey folks, considering that inside an sidebar we can use collapses or anything I created this draft:

<ClaySidebar displayType="dark" position="left">
    <ClaySidebar.Buttons buttons={buttons} diplayType="tabs?" />
    <ClaySidebar.Contents>
        <WidgetsSidebarContent id="widgets" label="Widgets"/>
        <ApplicationsSidebarContent id="apps" label="Applications"/>
        <FragmentsSidebarContent id="apps" label="Applications"/>
    </ClaySidebar.Contents>
</ClaySidebar>


const buttons = 
[
    {
        icon: 'square',
        id: 'widgets',
        renderIcon?: (children, openCallback) => (<div className="bg-success" onClick={openCallback}> {children} </div>)
    },
    {
        icon: 'app',
        id: 'apps',
        label: 'Applications',
        renderIcon?: (children, openCallback) => (<div className="bg-success" onClick={openCallback}> {children} </div>)
    },
    {
        separator
    },
    {
        icon: 'fragments',
        id: 'fragments',
        label: 'Fragments',
        renderIcon?: (children, openCallback) => (<div className="bg-success" onClick={openCallback}> {children} </div>)
    },
]

Where buttons.id can be used to display the sidebar content.

What are you thoughts on this?

@bryceosterhaus
Copy link
Member

@diegonvs how do you see the id being used to display/hide the content? Generally I think we should avoid ids as a required prop since they must be unique on the page.

@diegonvs
Copy link
Contributor

diegonvs commented Feb 6, 2020

@diegonvs how do you see the id being used to display/hide the content?

It just tells to the ClaySidebar.Contents component:
"- Hey, the user selected this button, so, render me a sidebar body with this component."

The logic to show and hide the Sidebar will be handled by ClaySidebar component.

Generally I think we should avoid ids as a required prop since they must be unique on the page.

We could rename this property.

@jbalsas
Copy link
Contributor Author

jbalsas commented Feb 7, 2020

Going forward, I'm trying to address some of the concerns expressed in #1104. We should refrain from being too generic because modifying sidebar-dark would cause changes on admin related UI. It seems that is an unwanted behaviour for a lot of folks. We really should free up the generic namespaces for devs and scope our stuff to something a little more specific. The component name can be generic relative to Liferay Portal Admin in my opinion.

Hey @pat270, I think this is both the beauty and the curse of CSS, isn't it? :)

In general, we need to carefully analyze such reports so we don't put the bandaid where it's not needed. There's a lot of things to be considered and same way people expect some things not to change, other people expect the exact opposite.

In this case, as we move to SaaS, it's critical that we have a coherent set of components that all react cohesively to a small set of variations (skins). This is becoming increasingly important to our company as we try to make the jump to SaaS offerings like Commerce.

With all that in mind, as I recognize this is a real issue and other solutions might be harder to implement (shadow DOM) or more harmful to users (documentation), I think we could move with the .c- prefix for our components... but nothing with the -journal or -product-menu suffixes :)

@pat270
Copy link
Member

pat270 commented Feb 10, 2020

@jbalsas
.c- prefix is good, anything that will differentiate those. I still think we need specific classes for each bar. Clay doesn't need to add them, but whoever is making those specific tbar and sidebar should include them.

@diegonvs diegonvs assigned diegonvs and unassigned edalgrin Feb 12, 2020
pat270 added a commit to pat270/clay that referenced this issue Feb 25, 2020
… applying `hover`, `focus`, `disabled`, `active`, `active-class-after`, `active-focus`.

feat(clay-css): Mixins `clay-button-variant` adds option to configure `overflow`

fix(clay-css): Mixins `clay-button-variant` deprecate options `hover-bg`, `hover-border-color`, `hover-color`, `hover-opacity`, `hover-text-decoration`, `focus-bg`, `focus-border-color`, `focus-box-shadow`, `focus-color`, `focus-opacity`, `focus-outline`, `disabled-bg`, `disabled-border-color`, `disabled-box-shadow`, `disabled-color`, `disabled-cursor`, `disabled-opacity`, `active-bg`, `active-border-color`, `active-box-shadow`, `active-color`, `active-opacity`, `active-focus-box-shadow`

issue liferay#2903
pat270 added a commit to pat270/clay that referenced this issue Feb 25, 2020
…pplying base `tbar`, `strong`, `.tbar-item`, `.tbar-section` styles

feat(clay-css): Mixins `clay-tbar-variant` use `clay-button-variant` mixin for applying `.tbar-btn`, `.tbar-btn-monospaced` styles

feat(clay-css): Mixins `clay-tbar-variant` use `clay-link` mixin for applying `.tbar-link`, `.tbar-link-monospaced` styles

feat(clay-css): Mixins `clay-tbar-variant` add option to configure `.tbar-nav`, `.tbar-divider-before`, `.tbar-divider-after`, `tbar-item-expand` via Sass maps and `clay-css` mixin

feat(clay-css): Mixins `clay-tbar-variant` deprecate options `border-color`, `border-style`, `border-width`, `bg-color`, `color`, `font-size`, `height`, `padding-x`, `padding-y`, `strong-font-weight`, `item-justify-content`, `item-padding-x`, `item-padding-y`, `btn-height`, `btn-font-size`, `btn-font-weight`, `btn-line-height`, `btn-margin-x`, `btn-margin-y`, `btn-padding-x`, `btn-padding-y`, `btn-monospaced-border-radius`, `btn-monospaced-border-width`, `btn-monospaced-font-size`, `btn-monospaced-margin-x`, `btn-monospaced-margin-y`, `btn-monospaced-padding`, `btn-monospaced-size`, `link-margin-x`, `link-margin-y`, `link-padding-x`, `link-padding-y`, `link-monospaced-border-radius`, `link-monospaced-border-width`, `link-monospaced-font-size`, `link-monospaced-margin-x`, `link-monospaced-margin-y`, `link-monospaced-padding`, `link-monospaced-size`, `section-text-align`

issue liferay#2903
pat270 added a commit to pat270/clay that referenced this issue Feb 25, 2020
…Pattern and variants `.tbar-light`, `.tbar-dark-l2`, .tbar-dark-d1`

feat(clay-css): Tbar adds Sass maps  `$tbar-stacked`, `$tbar-light`, `$tbar-dark-d1`, `$tbar-dark-l2`

issue liferay#2903
pat270 added a commit to pat270/clay that referenced this issue Feb 25, 2020
… `.sidebar`, `.sidebar-panel`, `.sidebar-header`, `.sidebar-body`, `.sidebar-footer`, `.nav-nested`, `.sidebar-list-group .list-group-item`, `.sidebar-list-group .sticker`, `.sidebar-list-group .sticker-secondary` via Sass maps and `clay-css` mixin

feat(clay-css): Mixins `clay-sidebar-variant` add option to configure `.close` via Sass map and `clay-close` mixin

feat(clay-css): Mixins `clay-sidebar-variant` add option to configure `.nav-nested .nav-link` via Sass map and `clay-link` mixin

issue liferay#2903
pat270 added a commit to pat270/clay that referenced this issue Feb 25, 2020
…idebar-dark` which uses `clay-sidebar-variant` mixin

issue liferay#2903
marcoscv-work pushed a commit that referenced this issue Mar 6, 2020
…ut-transition-in`, `.c-slideout-transition-out`, `.c-slideout-fixed`, `.c-slideout-absolute`, `.c-slideout-start`, `.c-slideout-end`

feat(clay-css): Slideout adds variables `$c-slideout-sidebar-width`, `$c-slideout-sidebar-width-mobile`, `$c-slideout-tbar-stacked-min-width`, `$c-slideout-tbar-stacked-min-width-mobile`

feat(clay-css): Slideout adds Sass maps `$c-slideout-transition-in`, `$c-slideout-transition-out`, `$c-slideout-absolute`, `$c-slideout-fixed`, `$c-slideout`, `$c-slideout-sm-down`, `$c-slideout-start`, `$c-slideout-start-sm-down`, `$c-slideout-end`, `$c-slideout-end-sm-down`

issue #2903
marcoscv-work pushed a commit that referenced this issue Mar 6, 2020
…, `focus-z-index`, `disabled-z-index`, `active-z-index` and use the Sass maps instead

issue #2903
@jbalsas
Copy link
Contributor Author

jbalsas commented Apr 23, 2020

Removing from the milestone for now, since this will likely take additional time

bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
… applying `hover`, `focus`, `disabled`, `active`, `active-class-after`, `active-focus`.

feat(clay-css): Mixins `clay-button-variant` adds option to configure `overflow`

fix(clay-css): Mixins `clay-button-variant` deprecate options `hover-bg`, `hover-border-color`, `hover-color`, `hover-opacity`, `hover-text-decoration`, `focus-bg`, `focus-border-color`, `focus-box-shadow`, `focus-color`, `focus-opacity`, `focus-outline`, `disabled-bg`, `disabled-border-color`, `disabled-box-shadow`, `disabled-color`, `disabled-cursor`, `disabled-opacity`, `active-bg`, `active-border-color`, `active-box-shadow`, `active-color`, `active-opacity`, `active-focus-box-shadow`

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…pplying base `tbar`, `strong`, `.tbar-item`, `.tbar-section` styles

feat(clay-css): Mixins `clay-tbar-variant` use `clay-button-variant` mixin for applying `.tbar-btn`, `.tbar-btn-monospaced` styles

feat(clay-css): Mixins `clay-tbar-variant` use `clay-link` mixin for applying `.tbar-link`, `.tbar-link-monospaced` styles

feat(clay-css): Mixins `clay-tbar-variant` add option to configure `.tbar-nav`, `.tbar-divider-before`, `.tbar-divider-after`, `tbar-item-expand` via Sass maps and `clay-css` mixin

feat(clay-css): Mixins `clay-tbar-variant` deprecate options `border-color`, `border-style`, `border-width`, `bg-color`, `color`, `font-size`, `height`, `padding-x`, `padding-y`, `strong-font-weight`, `item-justify-content`, `item-padding-x`, `item-padding-y`, `btn-height`, `btn-font-size`, `btn-font-weight`, `btn-line-height`, `btn-margin-x`, `btn-margin-y`, `btn-padding-x`, `btn-padding-y`, `btn-monospaced-border-radius`, `btn-monospaced-border-width`, `btn-monospaced-font-size`, `btn-monospaced-margin-x`, `btn-monospaced-margin-y`, `btn-monospaced-padding`, `btn-monospaced-size`, `link-margin-x`, `link-margin-y`, `link-padding-x`, `link-padding-y`, `link-monospaced-border-radius`, `link-monospaced-border-width`, `link-monospaced-font-size`, `link-monospaced-margin-x`, `link-monospaced-margin-y`, `link-monospaced-padding`, `link-monospaced-size`, `section-text-align`

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…Pattern and variants `.tbar-light`, `.tbar-dark-l2`, .tbar-dark-d1`

feat(clay-css): Tbar adds Sass maps  `$tbar-stacked`, `$tbar-light`, `$tbar-dark-d1`, `$tbar-dark-l2`

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
… `.sidebar`, `.sidebar-panel`, `.sidebar-header`, `.sidebar-body`, `.sidebar-footer`, `.nav-nested`, `.sidebar-list-group .list-group-item`, `.sidebar-list-group .sticker`, `.sidebar-list-group .sticker-secondary` via Sass maps and `clay-css` mixin

feat(clay-css): Mixins `clay-sidebar-variant` add option to configure `.close` via Sass map and `clay-close` mixin

feat(clay-css): Mixins `clay-sidebar-variant` add option to configure `.nav-nested .nav-link` via Sass map and `clay-link` mixin

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…idebar-dark` which uses `clay-sidebar-variant` mixin

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…onfigure `c-slideout-shown`, `c-slideout-tbar-shown`, `c-slideout-tbar-shown-sidebar`, `sidebar`, `sidebar-c-slideout-show`, `sidebar-c-slideout-transition`, `tbar-stacked`, `tbar-stacked-c-slideout-show`, `tbar-stacked-c-slideout-transition`

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…ut-transition-in`, `.c-slideout-transition-out`, `.c-slideout-fixed`, `.c-slideout-absolute`, `.c-slideout-start`, `.c-slideout-end`

feat(clay-css): Slideout adds variables `$c-slideout-sidebar-width`, `$c-slideout-sidebar-width-mobile`, `$c-slideout-tbar-stacked-min-width`, `$c-slideout-tbar-stacked-min-width-mobile`

feat(clay-css): Slideout adds Sass maps `$c-slideout-transition-in`, `$c-slideout-transition-out`, `$c-slideout-absolute`, `$c-slideout-fixed`, `$c-slideout`, `$c-slideout-sm-down`, `$c-slideout-start`, `$c-slideout-start-sm-down`, `$c-slideout-end`, `$c-slideout-end-sm-down`

issue liferay#2903
bryceosterhaus pushed a commit to bryceosterhaus/clay that referenced this issue May 19, 2020
…, `focus-z-index`, `disabled-z-index`, `active-z-index` and use the Sass maps instead

issue liferay#2903
@brunofernandezg brunofernandezg assigned diegonvs and unassigned diegonvs Feb 15, 2021
@diegonvs diegonvs removed their assignment Feb 15, 2021
@pat270
Copy link
Member

pat270 commented Jul 28, 2021

Is this one still valid? The component teams have already created their sidebars in DXP. Product Menu sidebar was replaced with Application Menu. Feel free to reopen if it is.

@pat270 pat270 closed this as completed Jul 28, 2021
@drakonux
Copy link
Member

We created the vertical bar time ago to solve all the cases in DXP.

Is still valid? I would say yes, it's still valid and we should create the component and replace all the bars in DXP with a vertical bar component (customized in some cases). But I'm not the owner of DXP... nor do I know the plans of FI to regularize its use... so I summon you @nhpatt @dsanz

@drakonux
Copy link
Member

In fact, Corbin asked for this time ago see #3959

@pat270
Copy link
Member

pat270 commented Jul 30, 2021

@drakonux I will reopen

@pat270 pat270 reopened this Jul 30, 2021
@javiergamarra
Copy link

Yes, this is still valid as there is a definition in lexicon that is not implemented in Clay (just one, tables and treeview does not exist :P)

@github-actions
Copy link

github-actions bot commented Jun 6, 2022

This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-155343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) comp: clay-css Issues related to Clay CSS type: enhancement Issues that are open to bring improvements or refinement of code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants