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(data-grid) Add actions cell type #444

Merged
merged 8 commits into from
Jul 16, 2021
Merged

feat(data-grid) Add actions cell type #444

merged 8 commits into from
Jul 16, 2021

Conversation

jano314159
Copy link
Contributor

No description provided.

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Thank you @sagi-t this is great!

My only comment is regarding a little change in the CSS.

And also, I would ask you to run the code formatter too (you can do yarn format from the root folder or from packages/components).

Before this is ready to merge, we need to update the documentation (Storybook). There's some info regarding cell types in packages/storybook-vue/stories/3_components/data-grid/DataGrid.stories.mdx

Is this something you'd like to give it a try? I can also help, just let me know.

Thanks again!

packages/components/src/components/data-grid/data-grid.css Outdated Show resolved Hide resolved
@acstll
Copy link
Collaborator

acstll commented Jul 15, 2021

@sagi-t thanks again for the update!

We were discussing yesterday that it would be nice to offer the possibility also to have buttons with icons or icon-only buttons.

I thought about this for the implementation:

const { label, ...props } = action;
+ if (typeof label === 'object' && '__html' in label) {
+   return <scale-button innerHTML={label.__html} {...props}></scale-button>;
+ }
return <scale-button {...props}>{label}</scale-button>;

so one can replace the label with some HTML, but we still keep label as the only prop in the row object not going into the button as attribute, which I think is important to keep the API clean and consistent.

{
  label: {
    __html: `<scale-icon-action-edit></scale-icon-action-edit>`
  },
  variant: 'secondary',
  iconOnly: 'true',
  ariaLabel: 'Edit',
  onClick: () => { /* ... */ },
}

Do you agree? would you do it differently?

@jano314159
Copy link
Contributor Author

Hi @acstll. No I don't, I totally agree with you because these options are very important in this concept and also for us, We just didn't think of it :D So thank you. I will make the change as soon as possible.

if it is okay for you. :)

@acstll
Copy link
Collaborator

acstll commented Jul 15, 2021

if it is okay for you. :)

totally OK 👍

I will then take care of the documentation part afterwards.

Thanks again!

@jano314159
Copy link
Contributor Author

Thank you @acstll
I'm Finished. We are looking forward to the documentation part from your side :D
After is it merged when can we expect this to be released? We would like to use this component as soon as possible 💯

@acstll
Copy link
Collaborator

acstll commented Jul 15, 2021

thank you @sagi-t!

I'll take care of the docs in a different PR.

We can make a release tomorrow, with the feature undocumented, so you can start using it right away :)

The prettier check is still failing (which is weird, since you already did that in 78ec7b8); could you please try and run yarn format once more inside packages/components/?

image

@jano314159
Copy link
Contributor Author

thank you @sagi-t!

I'll take care of the docs in a different PR.

We can make a release tomorrow, with the feature undocumented, so you can start using it right away :)

The prettier check is still failing (which is weird, since you already did that in 78ec7b8); could you please try and run yarn format once more inside packages/components/?

image

Yes of course :)

@jano314159
Copy link
Contributor Author

thank you @sagi-t!
I'll take care of the docs in a different PR.
We can make a release tomorrow, with the feature undocumented, so you can start using it right away :)
The prettier check is still failing (which is weird, since you already did that in 78ec7b8); could you please try and run yarn format once more inside packages/components/?
image

Yes of course :)

Hmm🧐. Interesting I did that a second ago and everything is fine nothing changes on the files.
Am I do something wrong? :D
image

@acstll
Copy link
Collaborator

acstll commented Jul 16, 2021

Hmm🧐. Interesting I did that a second ago and everything is fine nothing changes on the files.
Am I do something wrong? :D

that is weird indeed, can you try with yarn instead of npm? (we use Yarn v1) — that makes a difference sometimes 🤷
also, you can try yarn format directly from the packages/components directory, not from the root.

Let me know how that goes.

(I can still merge it and fix it later.)

Thank you 🙏

@jano314159
Copy link
Contributor Author

Hmm🧐. Interesting I did that a second ago and everything is fine nothing changes on the files.
Am I do something wrong? :D

that is weird indeed, can you try with yarn instead of npm? (we use Yarn v1) — that makes a difference sometimes 🤷
also, you can try yarn format directly from the packages/components directory, not from the root.

Let me know how that goes.

(I can still merge it and fix it later.)

Thank you 🙏

Yay 🥶 So now I used yarn format instead of npm in the packages/components directory as you can see the picture below 👇

image

But nothing has changed 😮‍💨 Sorry for that @acstll. Any idea for the possible solution ?
I'm looking forward to your response and thank you for your patience 🙏

@acstll
Copy link
Collaborator

acstll commented Jul 16, 2021

No worries, I'm gonna merge and then re-check if the prettier issue persist, and fix it in another PR :)

@acstll acstll merged commit bedf407 into telekom:main Jul 16, 2021
@acstll acstll mentioned this pull request Jul 16, 2021
@acstll
Copy link
Collaborator

acstll commented Jul 16, 2021

Thank you very much, once more @sagi-t

We'll make the release on Monday 🙏

acstll pushed a commit that referenced this pull request Jul 20, 2021
oddcelot pushed a commit that referenced this pull request Aug 10, 2021
oddcelot pushed a commit that referenced this pull request Aug 17, 2021
acstll added a commit that referenced this pull request Aug 25, 2021
* Get bernerslee ready for new infra (#435)

- staging server
- badge (new)
- s3 pipeline removed

* Feat/logo accessibility  (#437)

* refactor: accessibility title structure

* refactor: accessibility titles and roles

* feat: link storie with accessibility

* refactor(rating-stars): rework with input range approach

also make the whole thing more modular with parts

* feat(rating-stars): add half star rating

* feat(rating-stars): use correct aria value and add better half visuals

* refactor: add size, editable label; css

* feat: only first star can clear all stars

* refactor: prop renaming; remove watcher;

* test: writing tests

* refactor: renaming value to rating

* refactor: stories

* feat: implement onTouchEnd

* feat(rating-stars): add readonly state and a11y features

* feat(rating-stars): simplify revert logic / handle minRating input

* feat: add prop readonly

* feat: edit PR requests

* feat: firstStarSelected with a default value of false

* feat: input gets focus onClick

* feat(rating-stars): add sketch generation and adjust input handling

* fix: revert handmade sketch includes

* test: update snapshots

* feat(data-grid) add Actions cell type (#444)

* fix: readonly rating stars not being read in JAWS

* test: update snapshots

* test: fix rating stars e2e

* style: format

* chore(rating-stars): update symbol id mapping

removes handmade version and replaces them with the the generated version

* refactor(rating-stars): add disabled state label color

* refactor(sketch): remove icon symbol overrides from rating stars

* chore: update formatting

* fix: patch path vulneralbility

reosolves CodeQL report

* chore: update sketch includes

Co-authored-by: Arturo Castillo Delgado <ac@iconstorm.com>
Co-authored-by: Marvin_Laubenstein <82942834+marvinLaubenstein@users.noreply.github.com>
Co-authored-by: Stefan Kopco <mail@oddobsessions.com>
Co-authored-by: Christian Pajung <christian.pajung@telekom.de>
Co-authored-by: Arturo Castillo Delgado <arturo@arturu.com>
Co-authored-by: Sagi Janos <86777428+sagi-T@users.noreply.github.com>
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