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(styles): added an optional Calendar legend section [ci visual] #5707

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

alexhristov14
Copy link
Contributor

The styling needed for the calendar legend: SAP/fundamental-ngx#12403

Example:
Screenshot 2024-10-18 at 10 08 46 AM

Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for fundamental-styles ready!

Name Link
🔨 Latest commit 7694890
🔍 Latest deploy log https://app.netlify.com/sites/fundamental-styles/deploys/67376f9f81e943000874412e
😎 Deploy Preview https://deploy-preview-5707--fundamental-styles.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alexhristov14 alexhristov14 self-assigned this Oct 18, 2024
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

Screen.Recording.2024-10-21.at.2.45.31.PM.mov

On focus elements "jump". Element's states (hover, focus, active) should not affect the element's dimensions. In this case, the focus outline should be shown without a change in the element height, which causes the "jump"

packages/styles/src/calendar.scss Outdated Show resolved Hide resolved
packages/styles/src/calendar.scss Outdated Show resolved Hide resolved
Comment on lines 107 to 110
| \` fd-calendar--legend__item--circle\` | Makes the legend item representation a circle
| \` fd-calendar--legend__item--square\` | Makes the legend item representation a square
| \` fd-calendar--legend__item--text\` | Adds a descriptive title to the legend item
| \` fd-calendar--legend__item--text-description\` | Adds any additional information to the legend item
Copy link
Contributor

Choose a reason for hiding this comment

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

These class names are not following the BEM rules.
fd-calendar is the block element, because the entire element can be "extracted" from a project and used in another project, aka it's an independent unit. The elements inside this blocks have the double underscore in their class names.
For example: fd-calendar__row, fd-calendar__item, fd-calendar__date, etc.
When you look at these classes you can immediately tell that it's a row/item/date, etc. that belong to the calendar component (block).
The double dash notation is a modifier. If you have an item, or date, that are a variation of the common one, you use a modifier class to add specifics for it. For example, if the weekend dates and the work days have different background, then you do something like this:
fd-calendar__date fd-calendar__date--weekend or fd-calendar__date fd-calendar__date--work, fd-calendar__date fd-calendar__date--vacation.
When you read the class fd-calendar__date--vacation it basically says: "it's a vacation variation of date element that belongs to calendar".
If you have to read this class fd-calendar--legend__item--circle, you can't tell what is an element and what is a modifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 427 to 430
display: grid;
grid-template-columns: repeat(auto-fit, minmax(7.5rem, 1fr));
grid-gap: 0.5rem;
margin-top: 0.5rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is not part of the table, like I suggested in another comment, you can achieve this with flex. The reason to use flex here is because with modifier classes you can easily achieve different variations of the legend. It can be the way you did it, but it can also be like this:
Screenshot 2024-10-21 at 2 50 18 PM

Comment on lines 435 to 437
@include fd-flex();

align-items: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

@include fd-inline-flex-center()

border: 0.0625rem solid var(--sapContent_ForegroundBorderColor); // 0.125rem if not ForegroundBorderColor
border-radius: 0.125rem;
background-color: none;
margin: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

margin-inline: 0;
margin-block: 0;

&-description {
@include legendTextBase();

position: absolute;
Copy link
Contributor

Choose a reason for hiding this comment

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

absolute positioning should be your last option. Try with flex.

position: absolute;
vertical-align: middle;
white-space: pre-line;
text-overflow: ellipsis;
Copy link
Contributor

Choose a reason for hiding this comment

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

@mixin fd-ellipsis {
  white-space: nowrap;
  overflow: hidden;
  text-overflow: ellipsis;
}

white-space: pre-line;
text-overflow: ellipsis;
overflow: hidden;
padding-top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding-block-start: 0;

}

@include fd-focus(){
padding: 0.125rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding-inline: 0.125rem;
padding-block: 0.125rem;

@alexhristov14 alexhristov14 changed the title feat(styles): added an optional Calendar legend section [WIP] feat(styles): added an optional Calendar legend section Oct 25, 2024
@droshev droshev added this to the Sprint 139 - October 2024 milestone Oct 29, 2024
@alexhristov14 alexhristov14 changed the title [WIP] feat(styles): added an optional Calendar legend section feat(styles): added an optional Calendar legend section [ci visual] Oct 31, 2024
@InnaAtanasova InnaAtanasova force-pushed the feat/new-calendar-legend branch 4 times, most recently from 0140703 to bc91537 Compare November 14, 2024 20:10
@droshev droshev self-requested a review November 15, 2024 14:54
@InnaAtanasova InnaAtanasova merged commit 2d6941b into main Nov 15, 2024
10 checks passed
@InnaAtanasova InnaAtanasova deleted the feat/new-calendar-legend branch November 15, 2024 18:28
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.

3 participants