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

Same CSS rewritten for table and signal-viewer components #580

Open
PunitLodha opened this issue Dec 26, 2019 · 10 comments
Open

Same CSS rewritten for table and signal-viewer components #580

PunitLodha opened this issue Dec 26, 2019 · 10 comments

Comments

@PunitLodha
Copy link
Contributor

@domoritz Currently there is some CSS which is rewritten for both the table and signal-viewer components.

We can fix this by doing any of the following:-

  1. Removing the CSS in the signal-viewer component, so that the table component CSS can be used instead
  2. Changing the class name for the signal-viewer component(Currently both the components have a class name of "editor-table"), so that there is no overriding of styles
  3. Reusing the table component inside the signal-viewer component

Which one of the following methods should i use to fix this issue?

@domoritz
Copy link
Member

How are 1 and 3 different?

I’m happy to trust your judgement on what reduces duplication. Which option do you recommend?

@PunitLodha
Copy link
Contributor Author

1 is just removing the CSS from the signal-viewer component, so that the table component CSS is used(Due to the global nature of CSS). If you want to keep CSS separate for each of the components then it is not recommended.

3 is making use of the table component inside the signal-viewer component, similar to the data-viewer component using the table component. Currently the signal-viewer component uses Signal Row component to construct the table.Instead if we can use the table component, then there will be no need for extra CSS rules.

Personally I would recommend option 3, if its possible to implement it.

@domoritz
Copy link
Member

Please note #459. I’d like that pull request to get finished eventually and not create too many merge conflicts. Do you want to finish it first or make changes that do t conflict too much?

@domoritz
Copy link
Member

Cc @siddhant1

@PunitLodha
Copy link
Contributor Author

We should wait for #459 to finish

@domoritz
Copy link
Member

domoritz commented Jan 9, 2020

@siddhant1 What's your timeline for #459?

@siddhant1
Copy link
Member

It's complete , just need to add batching (maybe we can do it later)

@domoritz
Copy link
Member

Merged 🎉 . @PunitLodha Feel free to send PRs to clean up the css.

@PunitLodha
Copy link
Contributor Author

Is the signal viewer component static(unlike the timeline component)? If it is then i can try to reuse the table component in it

@domoritz
Copy link
Member

That would be nice. Thanks!

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

No branches or pull requests

3 participants