OSS::InformationSection component from 2FA branch #459

OwenCoogan · 2024-12-18T08:38:07Z

What does this PR do?

It adds a new component used in the 2FA project

Related to: #

What are the observable changes?

Good PR checklist

… doc

OSS::InformationSection component

JulienVannier66 · 2024-12-18T08:48:15Z

addon/components/o-s-s/information-section.hbs

@@ -0,0 +1,41 @@
+<div
+  data-control-name="information-section"
+  class="oss--information-section border-radius-md border-color-gray-200 box-shadow-md"


Suggested change

class="oss--information-section border-radius-md border-color-gray-200 box-shadow-md"

class="oss-information-section border-radius-md border-color-gray-200 box-shadow-md"

According to BEM -- is to modifier, maybe just use -

JulienVannier66 · 2024-12-18T08:50:20Z

addon/components/o-s-s/information-section.hbs

+>
+  <div
+    class="fx-row fx-malign-space-between fx-xalign-center padding-px-18 {{this.plainClass}}"
+    data-control-name="information-section--header"


Suggested change

data-control-name="information-section--header"

data-control-name="information-section-header"

We never use double dash in data-control-name, maybe just use single dash

JulienVannier66 · 2024-12-18T08:50:36Z

addon/components/o-s-s/information-section.hbs

+
+      <div class="fx-col fx-gap-px-3">
+        <span
+          data-control-name="information-section--title"


Suggested change

data-control-name="information-section--title"

data-control-name="information-section-title"

JulienVannier66 · 2024-12-18T08:50:45Z

addon/components/o-s-s/information-section.hbs

+        </span>
+
+        {{#if @subtitle}}
+          <span data-control-name="information-section--subtitle" class="font-color-gray-500">{{@subtitle}}</span>


Suggested change

<span data-control-name="information-section--subtitle" class="font-color-gray-500">{{@subtitle}}</span>

<span data-control-name="information-section-subtitle" class="font-color-gray-500">{{@subtitle}}</span>

JulienVannier66 · 2024-12-18T08:50:57Z

addon/components/o-s-s/information-section.hbs

+    {{yield to="action"}}
+  </div>
+  {{#if (has-block "content")}}
+    <hr class="oss--information-section__separator" />


Suggested change

<hr class="oss--information-section__separator" />

<hr class="oss-information-section__separator" />

JulienVannier66 · 2024-12-18T08:54:29Z

addon/components/o-s-s/information-section.stories.js

+export default {
+  title: 'Components/OSS::InformationSection',
+  argTypes: {
+    title: {


For title, according to the TS typing, can you add type: { required: true }

Added additional param

JulienVannier66 · 2024-12-18T08:57:11Z

addon/components/o-s-s/information-section.ts

+
+export default class OSSInformationSection extends Component<InformationSectionArgs> {
+  get plainClass(): string {
+    return this.args.plain === false ? 'background-color-gray-50' : 'background-color-white';


For undefined value, you want the plain effect? Like the true value?

Yep, thats it, plain === true would mean the header & content have a white background, plain === false would only have the header in gray. plain === undefined defaults out to all white

phndiaye · 2024-12-18T08:44:14Z

addon/components/o-s-s/information-section.hbs

@@ -0,0 +1,41 @@
+<div
+  data-control-name="information-section"
+  class="oss--information-section border-radius-md border-color-gray-200 box-shadow-md"


just oss-information-section as it's not a bem modifier but the base class. and also, if you're going to have class here, why not move all the others in its rules?

Moved all to css

phndiaye · 2024-12-18T08:47:17Z

addon/components/o-s-s/information-section.ts

+import type { IconNames } from './iconName.enum';
+
+interface InformationSectionArgs {
+  badgeIcon?: keyof typeof IconNames;


not sure about the typing here, this was an attempt but we ended up never being able to really use it as it doesn't really cover all font awesome. can't it be just string?
and also there is still no way to really enforce it in templates

moved to a string type

phndiaye · 2024-12-18T08:48:30Z

addon/components/o-s-s/information-section.ts

+import Component from '@glimmer/component';
+import type { IconNames } from './iconName.enum';
+
+interface InformationSectionArgs {


can you group all mandatory args together, and all optional ones together ? :)

Moved mandatory arg to the top

phndiaye · 2024-12-18T08:50:29Z

addon/components/o-s-s/information-section.hbs

+      {{#if @badgeIcon}}
+        <OSS::Badge @icon={{@badgeIcon}} class="border-color-primary-200" />
+      {{else if @imageUrl}}
+        <OSS::Badge @image={{@imageUrl}} class="border-color-primary-200" />
+      {{else}}
+        {{yield to="badge"}}
+      {{/if}}


i don't understand the forced border update here...
also we yield to badge meaning someone could decide to use a custom badge skin just to discover this line breaks it

Made workaround to be able to have the blue border on default badges without overriding content block

phndiaye · 2024-12-18T08:59:24Z

addon/components/o-s-s/information-section.hbs

global comment, i see a lot of data-control-name attributes that look a lot like classes, i don't remember seeing this much granularity in data-control-names considering it's usually really contextual and i have a mixed feeling about that 🤔

left data control name for parent div only

phndiaye · 2024-12-18T09:00:12Z

addon/components/o-s-s/information-section.hbs

+  ...attributes
+>
+  <div
+    class="fx-row fx-malign-space-between fx-xalign-center padding-px-18 {{this.plainClass}}"


if we are going to have a getter in it, why not just do a computedClasses getter that have all of them + the conditional ones and Array#join them?

Didn't see the need to array join but the full string is returned in a headerComputedClasses getter

OwenCoogan and others added 7 commits December 16, 2024 15:45

feat: added oss::information-section component with tests & storybook…

d605d81

… doc

fix: updates post review

5642f64

fix: updates post review

10fc6d9

fix: added fixes post review

0e4f9fe

fix: fixes post review

bae4d02

fix: added post review fixes

c1b61f0

Merge pull request #457 from upfluence/oc/DRA-2041

2d4ea4f

OSS::InformationSection component

upfluence-bot added the reviewed/needs-review label Dec 18, 2024

Miexil approved these changes Dec 18, 2024

View reviewed changes

upfluence-bot added reviewed/ready-to-merge and removed reviewed/needs-review labels Dec 18, 2024

JulienVannier66 reviewed Dec 18, 2024

View reviewed changes

upfluence-bot added reviewed/needs-work and removed reviewed/ready-to-merge labels Dec 18, 2024

phndiaye reviewed Dec 18, 2024

View reviewed changes

upfluence-bot added reviewed/needs-work and removed reviewed/needs-work labels Dec 18, 2024

OwenCoogan added 2 commits December 18, 2024 10:25

fix: added fixes post review

c301655

fix: added fixes post review

d85c4fc

upfluence-bot added reviewed/needs-review reviewed/needs-work and removed reviewed/needs-work reviewed/needs-review labels Dec 18, 2024

upfluence-bot added reviewed/needs-work and removed reviewed/needs-work labels Dec 18, 2024

phndiaye approved these changes Dec 18, 2024

View reviewed changes

upfluence-bot added reviewed/ready-to-merge and removed reviewed/needs-work labels Dec 18, 2024

OwenCoogan merged commit 9e5c64e into master Dec 18, 2024
3 checks passed

OwenCoogan deleted the feature-branch/2fa branch December 18, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSS::InformationSection component from 2FA branch #459

OSS::InformationSection component from 2FA branch #459

OwenCoogan commented Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

JulienVannier66 Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

phndiaye Dec 18, 2024

OwenCoogan Dec 18, 2024

	class="oss--information-section border-radius-md border-color-gray-200 box-shadow-md"
	class="oss-information-section border-radius-md border-color-gray-200 box-shadow-md"

	data-control-name="information-section--header"
	data-control-name="information-section-header"

	data-control-name="information-section--title"
	data-control-name="information-section-title"

	<span data-control-name="information-section--subtitle" class="font-color-gray-500">{{@subtitle}}</span>
	<span data-control-name="information-section-subtitle" class="font-color-gray-500">{{@subtitle}}</span>

	<hr class="oss--information-section__separator" />
	<hr class="oss-information-section__separator" />

OSS::InformationSection component from 2FA branch #459

OSS::InformationSection component from 2FA branch #459

Conversation

OwenCoogan commented Dec 18, 2024

What does this PR do?

What are the observable changes?

Good PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment