changed position to 'relative'

[ffknob]

Summary

Changed the position value of the euiScreenReaderOnly class from absolute to inherit .

Checklist

• Checked in dark mode
• Checked in mobile
• Checked in IE11 and Firefox
• Props have proper autodocs
• Added documentation examples
• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[snide]

What is the reasoning for this change? This content is visually hidden by default, and it makes sense to do as much as we can to make sure it doesn't interfere with visible content in the dom.

[ffknob]

What is the reasoning for this change? This content is visually hidden by default, and it makes sense to do as much as we can to make sure it doesn't interfere with visible content in the dom.

It may not be the way to solve #2477 , but I was aiming at making the <caption> not to have any visual aspect (if you take a look at how it is now it doubles the height of the table header line).

[ffknob]

yeah, I think I jumped to quickly in this one...
But if you could point me the way to go I'd be happy to help.

[cchaos]

I think it may be the "right" fix but not where we'd want the fix to happen since this change would affect all instances of EuiScreenReaderOnly instead of just the table caption. The EuiScreenReaderOnly component reduces the size of the contents to just 1px so it wouldn't be very detrimental to just change the position property of this one instance.

However, this would mean, we probably should use the SASS mixin instead of the React component where we could then effectively override position property.

[ffknob]

I made a new attempt. Now I change the position of the caption inside the scss for the basic table component. I don't know if that was what you meant, please let me know...

(by the way, I don't have that much experience with React and SCSS, trying to learn helping Elastic... so please forgive me for the noob mistakes and any help/directions will be much appreciated)

[snide]

so please forgive me for the noob mistakes and any help/directions will be much appreciated

No apologies necessary. We appreciate any outside contributions and will help out as we can.

One of our general rules is to not use "naked" selectors (meaning caption by itself in the sass file). The reason is that you never know what might else use that selector (even if it is scoped by its parent). General practice is to add a a css selector on the element itself so it has a unique purpose. Something like euiTableCaption. You'd add it to this file https://github.com/elastic/eui/blob/master/src/components/table/_table.scss rather than EuiBasicTable because the caption could be used as a separated usage beyond the high-order EuiBasicTable version.

Hope that makes sense. Yell if it doesn't and we can provide an example to learn.

[cchaos]

I think the intention is correct here, but we try to reduce the number of plain element selectors like this by using class names instead. To be sure we don't accidentally change a consumer's intended caption element, can you add a class name to this element and target it here.

eui/src/components/basic_table/basic_table.js

Line 467 in d5599a4

<caption role="status" aria-relevant="text" aria-live="polite">

So that line above would become something like

<caption className="euiBasicTable__caption" role="status" aria-relevant="text" aria-live="polite">

[cchaos]

Whoops, sorry for the double response. 😆

[ffknob]

Thank you guys so much for the guidelines. One thing is to know how to simply use some tech (e.g. SCSS), another completly different thing is to know how to properly used it (patterns, best practices, ...)! +1 for both explanations!

So, as I understood I could follow two approachs here:

snide's: define a more high level style (euiTableCaption in table/_table.scss) and apply it to the caption element. (this way it is available for both tables components)

or

cchao's: same idea, but instead define the element euiBasicTable__caption at the BasicTable's level. (this way it would only be available for the Basic Table component)

I decided to take the first road since it looks like it could be more easily used in other use case and also because @snide was the first one to reply...

But let me know if you guys prefer the second approach and I'll fix it!

Thanks for the very didactic feedback!

[snide]

See comment, which I think is slightly better and an easy change. The rest of your changes look good. Don't forget to edit the changelog. Be mindful of the past tense case it is written in. You want to add your change log entry under master.

On those changes this looks mergeable. TY for you contribution.

[ffknob]

See comment, which I think is slightly better and an easy change. The rest of your changes look good. Don't forget to edit the changelog. Be mindful of the past tense case it is written in. You want to add your change log entry under master.

On those changes this looks mergeable. TY for you contribution.

I notice that CHANGELOG has changed since I first cloned the repo.

Can you help me telling what would be the correct way to fetch those changes to my feature branch? (last time I tried to do something like this in a PR for the Kibana project I went from 1 file changed to +1800)

My plan was to:

my-branch$ git checkout master
master$ git remote add upstream git@github.com:elastic/eui.git
master$ git fetch upstream
master$ git merge upstream/master
master$ git checkout my-branch
my-branch$ git merge master 

Is that it?

[ffknob]

See comment, which I think is slightly better and an easy change. The rest of your changes look good. Don't forget to edit the changelog. Be mindful of the past tense case it is written in. You want to add your change log entry under master.
On those changes this looks mergeable. TY for you contribution.

I notice that CHANGELOG has changed since I first cloned the repo.

Can you help me telling what would be the correct way to fetch those changes to my feature branch? (last time I tried to do something like this in a PR for the Kibana project I went from 1 file changed to +1800)

My plan was to:

my-branch$ git checkout master
master$ git remote add upstream git@github.com:elastic/eui.git
master$ git fetch upstream
master$ git merge upstream/master
master$ git checkout my-branch
my-branch$ git merge master 

Is that it?

Never mind... it looks like it was it after all...

So, I included the change in changelog and changed from relative to fixed kept relative since fixed didn't seem to solve the issue.

[cchaos]

Thanks @ffknob ! Sorry it took so long to approve, the team has been away all week. There's just an issue with the Changelog entry. But once that's fixed. We'll get it merged.

[cchaos]

Jenkins, test this