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

feat(ui5-bar): initial implementation #2533

Merged
merged 13 commits into from
Dec 7, 2020

Conversation

tsanislavgatev
Copy link
Contributor

@tsanislavgatev tsanislavgatev commented Dec 1, 2020

FIXES #2434

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2020

CLA assistant check
All committers have signed the CLA.

packages/main/src/Bar.js Outdated Show resolved Hide resolved
packages/main/src/Bar.js Outdated Show resolved Hide resolved
packages/main/src/Bar.js Outdated Show resolved Hide resolved
packages/main/src/Bar.hbs Outdated Show resolved Hide resolved
packages/main/src/Bar.js Outdated Show resolved Hide resolved
Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, Tsani!

I have a couple of high-level remarks:

  1. Although Bar is in sap.m, for the webcomponents it logically belongs to the fiori package because this is a fiori/sap concept, and not something widely recognizable on the internet. Please move it.

  2. The sample should demonstrate also how we have the bar in the page. Or better, please use a bar inside the page sample ;)

  3. I would like more opinions on the usage. But I think it would be best to rework it so that we expect content directly in the 3 slots without a wrapper div.

packages/main/src/themes/Bar.css Outdated Show resolved Hide resolved
packages/main/src/themes/Bar.css Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let move the component to fiori, as the Page also will be in fiori, and this is a Fiori concept related component

@tsanislavgatev
Copy link
Contributor Author

tsanislavgatev commented Dec 1, 2020

Let move the component to fiori, as the Page also will be in fiori, and this is a Fiori concept related component

If we still don't have the page, should we stick to the Bar sample and then when the Page is implemented to add the Bar to the sample?
@ilhan007 @vladitasev

packages/fiori/src/Bar.js Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one samples_prepare.js file, where we should mark the Bar as new component by adding "Bar" in the newComponents array

packages/fiori/src/Bar.js Outdated Show resolved Hide resolved
packages/fiori/src/Bar.js Outdated Show resolved Hide resolved
packages/fiori/src/Bar.js Outdated Show resolved Hide resolved
packages/fiori/src/Bar.hbs Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ready to be merged, just few minor things left

@unazko
Copy link
Contributor

unazko commented Dec 4, 2020

Can we have a header and sub header section at the same time. It seems possible by visual specification.
It is showcased with buttons in the header section and a search field in the sub-header section.

@ilhan007
Copy link
Member

ilhan007 commented Dec 4, 2020

Can we have a header and sub header section at the same time. It seems possible by visual specification.
It is showcased with buttons in the header section and a search field in the sub-header section.

Yes, it is possible not only because of the visual spec, it is possible in general. The users can do everything, use arbitrary number of bars in different designs. Bars can be placed one below another like the Header + Subheader group, showed in the Visual spec. For this case I think we are fine as long as the users can remove the bar's border (box-shadow) with standard CSS and the default margin to remove the space between the bars.

ui5-bar {
  box-shadow: none;
  margin: 0
}

@ilhan007 ilhan007 dismissed their stale review December 7, 2020 13:19

outdated

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. The next step would be to create a BarDesign=Auto property and have the page set it (very similar to how the avatar group can set some properties to the avatar)

@ilhan007 ilhan007 merged commit 5c601cd into SAP:master Dec 7, 2020
ilhan007 pushed a commit that referenced this pull request Dec 7, 2020
Introduces a new component within the "fiori' package, called Bar (ui5-bar).
It serves as a container for buttons and similar components and it is mainly used for header or footer in application pages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Bar component
6 participants